Add x509 provider in authz #4
Conversation
remove new lines remove new lines
api.go
Outdated
| @@ -53,6 +55,8 @@ func GetLogger(ctx context.Context) Logger { | |||
| // IdentityToken provides an ntoken for Athenz access for the authorization handler itself. | |||
| type IdentityToken func() (string, error) | |||
|
|
|||
| type AthenzX509 func() (*tls.Config, error) | |||
There was a problem hiding this comment.
Would IdentityTLSCert or IdentityClientTLSCert be more appropriate?
There was a problem hiding this comment.
sounds good. just that it can get confusing with tls cert for webhook itself for https so I thought keeping x509 explicit might make more sense. thoughts?
There was a problem hiding this comment.
yep, but let's add Identity prefix to be consistent with IdentityToken as they are used for the same purpse
authz.go
Outdated
| for _, check := range checks { | ||
| client, err := a.client(ctx) | ||
| if a.AthenzX509 != nil { |
There was a problem hiding this comment.
AthenzX509 is a func type, what are we checking for nil here?
There was a problem hiding this comment.
If a struct property AthenzX509 is set here as a func then it should use x509 mode, else it would fallback on a token mode.
authz.go
Outdated
| xp509 := &http.Transport{ | ||
| TLSClientConfig: config, | ||
| } | ||
| debugXp := &debugTransport{} |
There was a problem hiding this comment.
Looks like we don't need this assignment here?
There was a problem hiding this comment.
@prabushyam I'm getting type mismatch otherwise.
api.go
Outdated
| @@ -119,6 +123,7 @@ type AuthorizationConfig struct { | |||
| Config // the base config | |||
| HelpMessage string // additional message for the user on internal authz errors | |||
| Token IdentityToken // the token provider for calls to Athenz | |||
| AthenzX509 AthenzX509 // the x509 provider for calls to Athenz | |||
There was a problem hiding this comment.
if we add a authorizationMode=TOKEN|TLS and call the appropriate client() method, would it be more straightforward?
There was a problem hiding this comment.
I was considering to add X509Mode as a boolean property so that it is easy to match instead of string-based flag. what do you think?
There was a problem hiding this comment.
X509Mode might be confusing when we add a feature flag to support user token/x509 certs. Let's add a prefix like Identity to this flag to be explicit about where it's used
|
@patelpayal Should we update the example https://github.com/yahoo/k8s-athenz-webhook/blob/master/example/auth-webhook/main.go too? |
|
@mcieplak I created an issue for #4 (comment) Here is the link: #5. |
No description provided.