Conversation
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
Author
VerificationSource tests and focused SONiC build checks passed. Environment
Source checks
SONiC checks
Verification boundary
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new authorization policy source backed directly by SONiC CONFIG_DB for grpc-go A43 method authorization, and refactors policy loading so telemetry/gnmi_server can install interceptors from either a watched policy file or a compiled in-memory policy.
Changes:
- Add
pkg/authzpolicywith pluggable policy sources (file,config_db) and deterministic compilation fromGRPC_AUTHZ_PRINCIPAL/GRPC_AUTHZ_RULE. - Extend
telemetryflags/config to select--authorization_policy_sourceand enforce verified client-cert transport when authz policy is enabled; propagate startup failures to a non-zero exit. - Update
gnmi_serverto load authz policy via the new source abstraction, close policy resources on shutdown, and disablegNSI Authz.RotatewhenCONFIG_DBowns policy.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| telemetry/telemetry.go | Adds policy-source flag/validation and propagates server startup errors to runTelemetry/main exit status. |
| telemetry/telemetry_test.go | Adds coverage for new exit behavior, startup-failure propagation, and policy-source flag handling. |
| pkg/authzpolicy/source.go | Introduces a policy source interface and file-based source preserving grpc-go file watcher behavior. |
| pkg/authzpolicy/source_test.go | Tests file-source loading and close idempotence plus invalid initial policy rejection. |
| pkg/authzpolicy/enforcement_test.go | End-to-end enforcement test for CONFIG_DB-compiled policy against unary and streaming RPCs. |
| pkg/authzpolicy/configdb.go | Implements CONFIG_DB table reading, policy compilation, and static interceptor creation/validation. |
| pkg/authzpolicy/configdb_test.go | Unit tests for table reading, determinism, validation failures, and source loading. |
| go.mod | Updates glog and adds explicit golang.org/x/time dependency. |
| go.sum | Records new module checksums. |
| gnmi_server/server.go | Switches from direct file watcher to loaded authzpolicy.PolicyInterceptor with proper shutdown cleanup. |
| gnmi_server/gnsi_authz.go | Disables Authz freshness/policy file handling and rejects Authz.Rotate in CONFIG_DB policy mode (after authn). |
| gnmi_server/authz_policy.go | Adds shared policy-loading helper and CONFIG_DB source wiring (Redis + sonic_db_config). |
| gnmi_server/authz_policy_test.go | Adds tests for policy source selection and CONFIG_DB Authz.Rotate authn-before-reject behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+146
to
+148
| if strings.HasPrefix(principal, "*") || strings.HasSuffix(principal, "*") { | ||
| return "", fmt.Errorf("principal %q must be a literal A43 principal", principal) | ||
| } |
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why I did it
In
sonic-gnmi, grpc-go A43 method authorization accepts only a policy file. Deployments that publish policy toCONFIG_DBneed a direct source. The source must not create an intermediate policy file.How I did it
Policy loading
filesource keeps the existing file watcher.config_dbsource readsGRPC_AUTHZ_PRINCIPALandGRPC_AUTHZ_RULE.config_dbsource expands roles into literal principals and creates deterministic A43 JSON in memory.sonic-gnmiusesauthz.NewStaticto validate the policy before installing unary and streaming interceptors.Startup and policy ownership
--authorization_policy_sourceacceptsfileandconfig_db. The default isfile.sonic-gnmirejects startup configurations that setinsecure,noTLS, orallow_no_client_auth.config_dbmode,sonic-gnmiauthenticates gNSIAuthz.Rotaterequests before rejecting them.GOPATH. This isolates tool installation from the inherited Go checksum cache.Authz.Rotaterejection preserves the CONFIG_DB publisher as the sole policy writer.runTelemetry.telemetryexits with a nonzero status.The
config_dbsource loads policy once during startup. Restart the service to apply policy changes.This PR does not configure a launcher to select
config_db. Live reload forconfig_dbrequires separate work. Atomic publication acrossGRPC_AUTHZ_PRINCIPALandGRPC_AUTHZ_RULEalso requires separate work.Launcher enablement, data processing unit forwarding, Unix domain socket enforcement, gNMI Pathz authorization, and image qualification are outside this PR.
How to verify it
make -f pure.mk ci PACKAGES=pkg/authzpolicypassed. Package coverage was 86.6%.go test -race -count=10 ./pkg/authzpolicypassed.go vet ./pkg/authzpolicy ./pkg/interceptorspassed.gnmi_serverandtelemetrywith generatedswsscommonandocbindscode.Authz.Rotateauthentication and rejection tests.GOPATH.Which release branch to backport
No release branch is selected.
Tested branch
Test result
master: All listed checks passed. Full image qualification is pending.Description for the changelog
Add a direct
CONFIG_DBsource for grpc-go A43 method authorization policies.Link to config_db schema for YANG module changes
This PR does not change YANG files. The proposed
CONFIG_DBschema is in sonic-buildimage PR #29087.A picture of a cute animal
Not included.