Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add x509 provider in authz #4

Merged
merged 6 commits into from
Jun 4, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"os"
"time"

"crypto/tls"

authn "k8s.io/api/authentication/v1beta1"
authz "k8s.io/api/authorization/v1beta1"
)
Expand Down Expand Up @@ -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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would IdentityTLSCert or IdentityClientTLSCert be more appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, but let's add Identity prefix to be consistent with IdentityToken as they are used for the same purpse


// AthenzPrincipal represents a valid Athenz principal.
type AthenzPrincipal struct {
Domain string // Athenz domain
Expand Down Expand Up @@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we add a authorizationMode=TOKEN|TLS and call the appropriate client() method, would it be more straightforward?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Mapper ResourceMapper // the resource mapper
}

Expand Down
28 changes: 27 additions & 1 deletion authz.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,27 @@ func (a *authorizer) client(ctx context.Context) (*client, error) {
return newClient(a.Endpoint, a.Timeout, xp), nil
}

// clientX509 returns the client set up with x509 cert and key to make calls to Athenz.
func (a *authorizer) clientX509(ctx context.Context) (*client, error) {

config, err := a.AthenzX509()
if err != nil {
return nil, err
}
xp509 := &http.Transport{
TLSClientConfig: config,
}
debugXp := &debugTransport{}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we don't need this assignment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prabushyam I'm getting type mismatch otherwise.

if isLogEnabled(ctx, LogTraceAthenz) {
debugXp = &debugTransport{
log: getLogger(ctx),
RoundTripper: xp509,
}
return newClient(a.Endpoint, a.Timeout, debugXp), nil
}
return newClient(a.Endpoint, a.Timeout, xp509), nil
}

// getSubjectAccessReview extracts the subject access review object from the request and returns it.
func (a *authorizer) getSubjectAccessReview(ctx context.Context, req *http.Request) (*authz.SubjectAccessReview, error) {
b, err := ioutil.ReadAll(req.Body)
Expand Down Expand Up @@ -131,8 +152,13 @@ func (a *authorizer) authorize(ctx context.Context, sr authz.SubjectAccessReview
}
internal := "internal setup error."
var via string
var client *client
for _, check := range checks {
client, err := a.client(ctx)
if a.AthenzX509 != nil {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AthenzX509 is a func type, what are we checking for nil here?

Copy link
Contributor Author

@patelpayal patelpayal May 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, thanks

client, err = a.clientX509(ctx)
} else {
client, err = a.client(ctx)
}
if err != nil {
return deny(NewAuthzError(err, internal), true)
}
Expand Down