Skip to content

Add ability to specify allowed CNs for RequestHeader proxy client cert #8443

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 12, 2016
Merged

Add ability to specify allowed CNs for RequestHeader proxy client cert #8443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 27 additions & 4 deletions pkg/auth/authenticator/request/x509request/x509.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@ package x509request
import (
"crypto/x509"
"crypto/x509/pkix"
"fmt"
"net/http"

"github.com/golang/glog"
"github.com/openshift/origin/pkg/auth/authenticator"
"k8s.io/kubernetes/pkg/auth/user"
kerrors "k8s.io/kubernetes/pkg/util/errors"
"k8s.io/kubernetes/pkg/util/sets"
)

// UserConversion defines an interface for extracting user info from a client certificate chain
Expand Down Expand Up @@ -68,10 +71,14 @@ func (a *Authenticator) AuthenticateRequest(req *http.Request) (user.Info, bool,
type Verifier struct {
opts x509.VerifyOptions
auth authenticator.Request

// allowedCommonNames contains the common names which a verified certificate is allowed to have.
// If empty, all verified certificates are allowed.
allowedCommonNames sets.String
}

func NewVerifier(opts x509.VerifyOptions, auth authenticator.Request) authenticator.Request {
return &Verifier{opts, auth}
func NewVerifier(opts x509.VerifyOptions, auth authenticator.Request, allowedCommonNames sets.String) authenticator.Request {
return &Verifier{opts, auth, allowedCommonNames}
}

// AuthenticateRequest verifies the presented client certificates, then delegates to the wrapped auth
Expand All @@ -82,8 +89,11 @@ func (a *Verifier) AuthenticateRequest(req *http.Request) (user.Info, bool, erro

var errlist []error
for _, cert := range req.TLS.PeerCertificates {
_, err := cert.Verify(a.opts)
if err != nil {
if _, err := cert.Verify(a.opts); err != nil {
errlist = append(errlist, err)
continue
}
if err := a.verifySubject(cert.Subject); err != nil {
errlist = append(errlist, err)
continue
}
Expand All @@ -92,6 +102,19 @@ func (a *Verifier) AuthenticateRequest(req *http.Request) (user.Info, bool, erro
return nil, false, kerrors.NewAggregate(errlist)
}

func (a *Verifier) verifySubject(subject pkix.Name) error {
// No CN restrictions
if len(a.allowedCommonNames) == 0 {
return nil
}
// Enforce CN restrictions
if a.allowedCommonNames.Has(subject.CommonName) {
return nil
}
glog.Warningf("x509: subject with cn=%s is not in the allowed list: %v", subject.CommonName, a.allowedCommonNames.List())
return fmt.Errorf("x509: subject with cn=%s is not allowed", subject.CommonName)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to assume that you're intentionally withholding the allowed set on the assumption that while a compromised signer means you're owned, they wouldn't be able to also trick openshift without either access to the master-config or the client cert from the existing proxy?

Seems a little thin to me, but if you want it that way, you should add a comment and maybe indicate where in the master-config they should look when they're rejected?

}

// DefaultVerifyOptions returns VerifyOptions that use the system root certificates, current time,
// and requires certificates to be valid for client auth (x509.ExtKeyUsageClientAuth)
func DefaultVerifyOptions() x509.VerifyOptions {
Expand Down
21 changes: 20 additions & 1 deletion pkg/auth/authenticator/request/x509request/x509_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/openshift/origin/pkg/auth/authenticator"
"k8s.io/kubernetes/pkg/auth/user"
"k8s.io/kubernetes/pkg/util/sets"
)

const (
Expand Down Expand Up @@ -536,6 +537,8 @@ func TestX509Verifier(t *testing.T) {

Opts x509.VerifyOptions

AllowedCNs sets.String

ExpectOK bool
ExpectErr bool
}{
Expand Down Expand Up @@ -579,6 +582,22 @@ func TestX509Verifier(t *testing.T) {
ExpectOK: true,
ExpectErr: false,
},
"valid client cert with wrong CN": {
Opts: getDefaultVerifyOptions(t),
AllowedCNs: sets.NewString("foo", "bar"),
Certs: getCerts(t, clientCNCert),

ExpectOK: false,
ExpectErr: true,
},
"valid client cert with right CN": {
Opts: getDefaultVerifyOptions(t),
AllowedCNs: sets.NewString("client_cn"),
Certs: getCerts(t, clientCNCert),

ExpectOK: true,
ExpectErr: false,
},

"future cert": {
Opts: x509.VerifyOptions{
Expand Down Expand Up @@ -614,7 +633,7 @@ func TestX509Verifier(t *testing.T) {
return &user.DefaultInfo{Name: "innerauth"}, true, nil
})

a := NewVerifier(testCase.Opts, auth)
a := NewVerifier(testCase.Opts, auth, testCase.AllowedCNs)

user, ok, err := a.AuthenticateRequest(req)

Expand Down
3 changes: 3 additions & 0 deletions pkg/cmd/server/api/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,9 @@ type RequestHeaderIdentityProvider struct {

// ClientCA is a file with the trusted signer certs. If empty, no request verification is done, and any direct request to the OAuth server can impersonate any identity from this provider, merely by setting a request header.
ClientCA string
// ClientCommonNames is an optional list of common names to require a match from. If empty, any client certificate validated against the clientCA bundle is considered authoritative.
ClientCommonNames []string

// Headers is the set of headers to check for identity information
Headers []string
// PreferredUsernameHeaders is the set of headers to check for the preferred username
Expand Down
1 change: 1 addition & 0 deletions pkg/cmd/server/api/v1/swagger_doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,7 @@ var map_RequestHeaderIdentityProvider = map[string]string{
"loginURL": "LoginURL is a URL to redirect unauthenticated /authorize requests to Unauthenticated requests from OAuth clients which expect interactive logins will be redirected here ${url} is replaced with the current URL, escaped to be safe in a query parameter\n https://www.example.com/sso-login?then=${url}\n${query} is replaced with the current query string\n https://www.example.com/auth-proxy/oauth/authorize?${query}",
"challengeURL": "ChallengeURL is a URL to redirect unauthenticated /authorize requests to Unauthenticated requests from OAuth clients which expect WWW-Authenticate challenges will be redirected here ${url} is replaced with the current URL, escaped to be safe in a query parameter\n https://www.example.com/sso-login?then=${url}\n${query} is replaced with the current query string\n https://www.example.com/auth-proxy/oauth/authorize?${query}",
"clientCA": "ClientCA is a file with the trusted signer certs. If empty, no request verification is done, and any direct request to the OAuth server can impersonate any identity from this provider, merely by setting a request header.",
"clientCommonNames": "ClientCommonNames is an optional list of common names to require a match from. If empty, any client certificate validated against the clientCA bundle is considered authoritative.",
"headers": "Headers is the set of headers to check for identity information",
"preferredUsernameHeaders": "PreferredUsernameHeaders is the set of headers to check for the preferred username",
"nameHeaders": "NameHeaders is the set of headers to check for the display name",
Expand Down
3 changes: 3 additions & 0 deletions pkg/cmd/server/api/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,6 +732,9 @@ type RequestHeaderIdentityProvider struct {

// ClientCA is a file with the trusted signer certs. If empty, no request verification is done, and any direct request to the OAuth server can impersonate any identity from this provider, merely by setting a request header.
ClientCA string `json:"clientCA"`
// ClientCommonNames is an optional list of common names to require a match from. If empty, any client certificate validated against the clientCA bundle is considered authoritative.
ClientCommonNames []string `json:"clientCommonNames"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of anything better.
.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might make it clientAuthorizedCommonNames to indicate that it's a list of the ones that will be accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might make it clientAuthorizedCommonNames to indicate that it's a list of the ones that will be accepted.

I think that's unnecessary. We don't specify the allowed side of an option unless we have a corresponding deny. Take the preceding option as an example. authorizedClientCA instead of clientCA is extraneous, because you don't make a blacklist of CAs (usually).

Copy link
Contributor

Choose a reason for hiding this comment

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

To me, clientCommonNames sounds like aliases for the client, not a list of acceptable clients. Also, a blacklist of common names is not necessarily a bad thing to have. For example, if there are other components of the network that should never be attempting this access, then blacklisting them could potentially thwart an attack.


// Headers is the set of headers to check for identity information
Headers []string `json:"headers"`
// PreferredUsernameHeaders is the set of headers to check for the preferred username
Expand Down
1 change: 1 addition & 0 deletions pkg/cmd/server/api/v1/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ oauthConfig:
apiVersion: v1
challengeURL: ""
clientCA: ""
clientCommonNames: null
emailHeaders: null
headers: null
kind: RequestHeaderIdentityProvider
Expand Down
3 changes: 3 additions & 0 deletions pkg/cmd/server/api/validation/oauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,10 @@ func ValidateRequestHeaderIdentityProvider(provider *api.RequestHeaderIdentityPr

if len(provider.ClientCA) > 0 {
validationResults.AddErrors(ValidateFile(provider.ClientCA, fieldPath.Child("provider", "clientCA"))...)
} else if len(provider.ClientCommonNames) > 0 {
validationResults.AddErrors(field.Invalid(fieldPath.Child("provider", "clientCommonNames"), provider.ClientCommonNames, "clientCA must be specified in order to use clientCommonNames"))
}

if len(provider.Headers) == 0 {
validationResults.AddErrors(field.Required(fieldPath.Child("provider", "headers"), ""))
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/cmd/server/origin/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,7 @@ func (c *AuthConfig) getAuthenticationRequestHandler() (authenticator.Request, e
return nil, fmt.Errorf("Error loading certs from %s: %v", provider.ClientCA, err)
}

authRequestHandler = x509request.NewVerifier(opts, authRequestHandler)
authRequestHandler = x509request.NewVerifier(opts, authRequestHandler, sets.NewString(provider.ClientCommonNames...))
}
authRequestHandlers = append(authRequestHandlers, authRequestHandler)

Expand Down
Loading