unify authn for xds and sds clients #45455
Conversation
There was a problem hiding this comment.
this is originally from pilot/pkg/xds/auth.go
There was a problem hiding this comment.
Not sure XDS and SDS/CA clients should have the same behavior.
For XDS - it is fine to use lighter auth, there is nothing secret - just configs and endpoints.
For SDS/CA - we want more strict auth. Unfortunately we don't do a lot of checks - like
verify the IP address ( now that we have the reverse index ), but should become stronger.
Not strongly against this change - IMO longer term it would be best to put a gateway in front of Istiod and use our own authn/authz instead of custom.
|
For SDS the strict part, we can pass in addional authentiocator. Or add a wrapper based on the shared function |
| "istio.io/istio/pkg/env" | ||
| ) | ||
|
|
||
| var AuthPlaintext = env.Register("XDS_AUTH_PLAINTEXT", false, |
There was a problem hiding this comment.
Making this apply to the CA is a major major security risk
There was a problem hiding this comment.
It does not change sds. YOu can see the return value of Authenticate for CA server is nil, nil when it is through 15010(plaintext)
But we donot allow nil for SDS
| if caller == nil { | ||
| return nil, status.Error(codes.Unauthenticated, "request authenticate failure") | ||
| return nil, status.Error(codes.Unauthenticated, err.Error()) |
There was a problem hiding this comment.
Not returning the error is intentional to avoid leaking sensitive info to unauthenticated callers
There was a problem hiding this comment.
This is the test, i can restore it too
|
@howardjohn ptaL |
pkg/security/authentication.go
Outdated
| // Authenticate authenticates the ADS request using the configured authenticators. | ||
| // Returns the validated principals or an error. | ||
| // If no authenticators are configured, or if the request is on a non-secure | ||
| // stream ( 15010 ) - returns an empty list of principals and no errors. |
There was a problem hiding this comment.
nit: "empty list of principals" should be updated, its now a nil struct
| caller := am.Authenticate(ctx) | ||
| if caller == nil { | ||
| serverCaLog.Errorf("Failed to authenticate client from %s: %s", peerAddr, am.FailedMessages()) | ||
| s.monitoring.AuthnError.Increment() |
Please provide a description of this PR:
To help us figure out who should review this PR, please put an X in all the areas that this PR affects.
Please check any characteristics that apply to this pull request.