From b36da40ab62aa59f977044316283291b041eda45 Mon Sep 17 00:00:00 2001 From: Charlie Egan Date: Thu, 9 Mar 2023 16:42:59 +0000 Subject: [PATCH] [server/identifier] Support SPIFFEID use in authz (#5742) When using OPA TLS authorization, authz policy authors will now have access to the client certificates presented as part of the TLS connection. This new data will be available under the key `client_certificates`. The existing functionality where `identity` is set to the Subject RDN Sequence is left unchanged. When using x.509 SVIDs the SPIFFEID is presented as in the SANs of the certificate. So this change makes that data available should users require it. It also makes other information about the client cert available to authz policy too. Signed-off-by: Charlie Egan --- server/authorizer/authorizer.go | 5 ++ server/identifier/certs.go | 25 ++++++ server/identifier/identifier.go | 38 ++------ server/identifier/mock_test.go | 21 +++++ .../identifier/testdata/spiffe-svid-cert.pem | 14 +++ .../identifier/testdata/spiffe-svid-key.pem | 5 ++ server/identifier/tls.go | 1 + server/identifier/tls_test.go | 90 ++++++++++++++----- server/identifier/token.go | 33 +++++++ .../{identifier_test.go => token_test.go} | 37 ++++---- 10 files changed, 197 insertions(+), 72 deletions(-) create mode 100644 server/identifier/certs.go create mode 100644 server/identifier/mock_test.go create mode 100644 server/identifier/testdata/spiffe-svid-cert.pem create mode 100644 server/identifier/testdata/spiffe-svid-key.pem create mode 100644 server/identifier/token.go rename server/identifier/{identifier_test.go => token_test.go} (61%) diff --git a/server/authorizer/authorizer.go b/server/authorizer/authorizer.go index 3778fe9e7b..225870a9ff 100644 --- a/server/authorizer/authorizer.go +++ b/server/authorizer/authorizer.go @@ -198,6 +198,11 @@ func makeInput(r *http.Request) (*http.Request, interface{}, error) { input["identity"] = identity } + clientCertificates, ok := identifier.ClientCertificates(r) + if ok { + input["client_certificates"] = clientCertificates + } + return r, input, nil } diff --git a/server/identifier/certs.go b/server/identifier/certs.go new file mode 100644 index 0000000000..17ad1ea80c --- /dev/null +++ b/server/identifier/certs.go @@ -0,0 +1,25 @@ +package identifier + +import ( + "context" + "crypto/x509" + "net/http" +) + +type clientCertificatesKey string + +const clientCertificates = clientCertificatesKey("org.openpolicyagent/client-certificates") + +// ClientCertificates returns the ClientCertificates of the caller associated with ctx. +func ClientCertificates(r *http.Request) ([]*x509.Certificate, bool) { + ctx := r.Context() + + certs, ok := ctx.Value(clientCertificates).([]*x509.Certificate) + + return certs, ok +} + +// SetClientCertificates returns a new http.Request with the ClientCertificates set to v. +func SetClientCertificates(r *http.Request, v []*x509.Certificate) *http.Request { + return r.WithContext(context.WithValue(r.Context(), clientCertificates, v)) +} diff --git a/server/identifier/identifier.go b/server/identifier/identifier.go index 6de3ad0d3a..de0c7dd82f 100644 --- a/server/identifier/identifier.go +++ b/server/identifier/identifier.go @@ -2,15 +2,18 @@ // Use of this source code is governed by an Apache2 // license that can be found in the LICENSE file. -// Package identifier provides handlers for associating an identity with incoming requests. +// Package identifier provides handlers for associating identity information with incoming requests. package identifier import ( "context" "net/http" - "regexp" ) +type identityKey string + +const identity = identityKey("org.openpolicyagent/identity") + // Identity returns the identity of the caller associated with ctx. func Identity(r *http.Request) (string, bool) { ctx := r.Context() @@ -25,34 +28,3 @@ func Identity(r *http.Request) (string, bool) { func SetIdentity(r *http.Request, v string) *http.Request { return r.WithContext(context.WithValue(r.Context(), identity, v)) } - -type identityKey string - -const identity = identityKey("org.openpolicyagent/identity") - -// TokenBased extracts Bearer tokens from the request. -type TokenBased struct { - inner http.Handler -} - -// NewTokenBased returns a new TokenBased object. -func NewTokenBased(inner http.Handler) *TokenBased { - return &TokenBased{ - inner: inner, - } -} - -var bearerTokenRegexp = regexp.MustCompile(`^Bearer\s+(\S+)$`) - -func (h *TokenBased) ServeHTTP(w http.ResponseWriter, r *http.Request) { - - value := r.Header.Get("Authorization") - if len(value) > 0 { - match := bearerTokenRegexp.FindStringSubmatch(value) - if len(match) > 0 { - r = SetIdentity(r, match[1]) - } - } - - h.inner.ServeHTTP(w, r) -} diff --git a/server/identifier/mock_test.go b/server/identifier/mock_test.go new file mode 100644 index 0000000000..58ff792ed6 --- /dev/null +++ b/server/identifier/mock_test.go @@ -0,0 +1,21 @@ +package identifier_test + +import ( + "crypto/x509" + "net/http" + + "github.com/open-policy-agent/opa/server/identifier" +) + +type mockHandler struct { + identity string + identityDefined bool + + clientCertificates []*x509.Certificate + clientCertificatesDefined bool +} + +func (h *mockHandler) ServeHTTP(_ http.ResponseWriter, r *http.Request) { + h.identity, h.identityDefined = identifier.Identity(r) + h.clientCertificates, h.clientCertificatesDefined = identifier.ClientCertificates(r) +} diff --git a/server/identifier/testdata/spiffe-svid-cert.pem b/server/identifier/testdata/spiffe-svid-cert.pem new file mode 100644 index 0000000000..a31fe5e4a0 --- /dev/null +++ b/server/identifier/testdata/spiffe-svid-cert.pem @@ -0,0 +1,14 @@ +-----BEGIN CERTIFICATE----- +MIICMjCCAdegAwIBAgIIKoc+GoX4gH8wCgYIKoZIzj0EAwIwXDELMAkGA1UEBhMC +R0IxFDASBgNVBAoTC0V4YW1wbGUgT3JnMRkwFwYDVQQLExBFeGFtcGxlIE9yZyBV +bml0MRwwGgYDVQQFExMxNTEwNjM0Mzk1MTQyOTY4MzA4MCAXDTIzMDIyMzE2NTYw +M1oYDzIxMjIwMTMwMTY1NjAzWjBcMQswCQYDVQQGEwJHQjEUMBIGA1UEChMLRXhh +bXBsZSBPcmcxGTAXBgNVBAsTEEV4YW1wbGUgT3JnIFVuaXQxHDAaBgNVBAUTEzMw +NjQ0ODYzNTUwODY2MzkyMzEwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATsy0fp +ZR6jixkkbWFEvjPhymHX5qo0w7FTfXEzYmmjF2neQv3IWRFCBwbyb5qn1sy0mlkv +MvZY/7KxqEBYf4YLo4GAMH4wDgYDVR0PAQH/BAQDAgeAMB0GA1UdJQQWMBQGCCsG +AQUFBwMCBggrBgEFBQcDATAMBgNVHRMBAf8EAjAAMD8GA1UdEQQ4MDaGNHNwaWZm +ZTovL2V4YW1wbGUuY29tL3Byb2QvZXUvY2x1c3Rlci9zZXJ2aWNlcy9mb29iYXIw +CgYIKoZIzj0EAwIDSQAwRgIhANmslXIVd++AgReE1oVUkCGQqSZLHs6F6Q4qS8Pb +/YAQAiEAy4FtOuqHyI2jKBZJjHXYYtE1mQPicAC9E1+fhLLRhg4= +-----END CERTIFICATE----- diff --git a/server/identifier/testdata/spiffe-svid-key.pem b/server/identifier/testdata/spiffe-svid-key.pem new file mode 100644 index 0000000000..1c3aee999a --- /dev/null +++ b/server/identifier/testdata/spiffe-svid-key.pem @@ -0,0 +1,5 @@ +-----BEGIN PRIVATE KEY----- +MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgC40SqDYRW5hWbr0P +ofGXXE355mXlYIEOSMWYCaE553OhRANCAATsy0fpZR6jixkkbWFEvjPhymHX5qo0 +w7FTfXEzYmmjF2neQv3IWRFCBwbyb5qn1sy0mlkvMvZY/7KxqEBYf4YL +-----END PRIVATE KEY----- diff --git a/server/identifier/tls.go b/server/identifier/tls.go index dc1373c6e6..d17c9f1314 100644 --- a/server/identifier/tls.go +++ b/server/identifier/tls.go @@ -24,6 +24,7 @@ func (h *TLSBased) ServeHTTP(w http.ResponseWriter, r *http.Request) { if tls := r.TLS; tls != nil { if certs := tls.PeerCertificates; len(certs) > 0 { r = SetIdentity(r, certs[0].Subject.ToRDNSequence().String()) + r = SetClientCertificates(r, certs) } } diff --git a/server/identifier/tls_test.go b/server/identifier/tls_test.go index 372d031ab3..d7257478f3 100644 --- a/server/identifier/tls_test.go +++ b/server/identifier/tls_test.go @@ -6,6 +6,8 @@ package identifier_test import ( "crypto/tls" + "crypto/x509" + "encoding/pem" "net/http" "net/http/httptest" "testing" @@ -22,33 +24,46 @@ func TestTLSBased(t *testing.T) { handler := identifier.NewTLSBased(mock) tests := []struct { - desc string - cert string - key string - expected string - defined bool + desc string + cert string + key string + identityExpected string + identityDefined bool + clientCertificatesDefined bool }{ { desc: "no cert", }, { - desc: "cert with CN=", - cert: "testdata/cn-cert.pem", - key: "testdata/key.pem", - expected: "CN=my-client", - defined: true, + desc: "cert with CN=", + cert: "testdata/cn-cert.pem", + key: "testdata/key.pem", + identityExpected: "CN=my-client", + identityDefined: true, + clientCertificatesDefined: true, }, { - desc: "cert with long DN", - cert: "testdata/ou-cert.pem", - key: "testdata/key.pem", - expected: "OU=opa-client-01,O=Torchwood", - defined: true, + desc: "cert with long DN", + cert: "testdata/ou-cert.pem", + key: "testdata/key.pem", + identityExpected: "OU=opa-client-01,O=Torchwood", + identityDefined: true, + clientCertificatesDefined: true, + }, + { + desc: "SPIFFE cert", + cert: "testdata/spiffe-svid-cert.pem", + key: "testdata/spiffe-svid-key.pem", + identityExpected: "SERIALNUMBER=3064486355086639231,OU=Example Org Unit,O=Example Org,C=GB", + identityDefined: true, + clientCertificatesDefined: true, }, } for _, tc := range tests { t.Run(tc.desc, func(t *testing.T) { + var err error + // Note: some re-use happens if this server is outside of the tests loop, // causing weird overlaps. Let's keep setting up a fresh one in each // iteration to be safe. @@ -58,26 +73,57 @@ func TestTLSBased(t *testing.T) { defer s.Close() c := s.Client() // trusts the httptest server's TLS cert + var cert tls.Certificate if tc.cert != "" && tc.key != "" { - cert, err := tls.LoadX509KeyPair(tc.cert, tc.key) + cert, err = tls.LoadX509KeyPair(tc.cert, tc.key) if err != nil { t.Fatalf("read test cert/key (%s/%s): %s", tc.cert, tc.key, err) } c.Transport.(*http.Transport).TLSClientConfig.Certificates = []tls.Certificate{cert} } - _, err := c.Get(s.URL) + _, err = c.Get(s.URL) if err != nil { t.Fatalf("unexpected error in GET %s: %s", s.URL, err) } - if mock.defined != tc.defined { - t.Fatalf("Expected defined to be %v but got: %v", tc.defined, mock.defined) + if mock.identityDefined != tc.identityDefined { + t.Fatalf("Expected identityDefined to be %v but got: %v", tc.identityDefined, mock.identityDefined) } - if mock.identity != tc.expected { - t.Fatalf("Expected identity to be %s but got: %s", tc.expected, mock.identity) + if tc.identityDefined { + if mock.identity != tc.identityExpected { + t.Fatalf("Expected identity to be %s but got: %s", tc.identityExpected, mock.identity) + } + } + + if mock.clientCertificatesDefined != tc.clientCertificatesDefined { + t.Fatalf("Expected clientCertificatesDefined to be %v but got: %v", tc.clientCertificatesDefined, mock.clientCertificatesDefined) + } + + if tc.clientCertificatesDefined { + if len(mock.clientCertificates) != 1 { + t.Fatalf("Expected clientCertificates to have 1 cert but got: %d", len(mock.clientCertificates)) + } + + gotPemData := pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: mock.clientCertificates[0].Raw, + }) + + parsedWantedCert, err := x509.ParseCertificate(cert.Certificate[0]) + if err != nil { + t.Fatalf("Error parsing expected cert: %s", err) + } + + wantPemData := pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: parsedWantedCert.Raw, + }) + + if got, want := string(gotPemData), string(wantPemData); got != want { + t.Fatalf("Expected clientCertificates to be \n%s\n but got: \n%s\n", want, got) + } } }) } - } diff --git a/server/identifier/token.go b/server/identifier/token.go new file mode 100644 index 0000000000..a5d57b38db --- /dev/null +++ b/server/identifier/token.go @@ -0,0 +1,33 @@ +package identifier + +import ( + "net/http" + "regexp" +) + +// TokenBased extracts Bearer tokens from the request. +type TokenBased struct { + inner http.Handler +} + +// NewTokenBased returns a new TokenBased object. +func NewTokenBased(inner http.Handler) *TokenBased { + return &TokenBased{ + inner: inner, + } +} + +var bearerTokenRegexp = regexp.MustCompile(`^Bearer\s+(\S+)$`) + +func (h *TokenBased) ServeHTTP(w http.ResponseWriter, r *http.Request) { + + value := r.Header.Get("Authorization") + if len(value) > 0 { + match := bearerTokenRegexp.FindStringSubmatch(value) + if len(match) > 0 { + r = SetIdentity(r, match[1]) + } + } + + h.inner.ServeHTTP(w, r) +} diff --git a/server/identifier/identifier_test.go b/server/identifier/token_test.go similarity index 61% rename from server/identifier/identifier_test.go rename to server/identifier/token_test.go index 1f1dcfad58..e315995349 100644 --- a/server/identifier/identifier_test.go +++ b/server/identifier/token_test.go @@ -11,15 +11,6 @@ import ( "github.com/open-policy-agent/opa/server/identifier" ) -type mockHandler struct { - identity string - defined bool -} - -func (h *mockHandler) ServeHTTP(_ http.ResponseWriter, r *http.Request) { - h.identity, h.defined = identifier.Identity(r) -} - func TestTokenBased(t *testing.T) { mock := &mockHandler{} @@ -31,13 +22,25 @@ func TestTokenBased(t *testing.T) { } tests := []struct { - value string - expected string - defined bool + value string + expected string + identityDefined bool }{ - {"", "", false}, - {"Bearer this-is-the-token", "this-is-the-token", true}, - {"Bearer this-is-the-token-with-spaces", "this-is-the-token-with-spaces", true}, + { + "", + "", + false, + }, + { + "Bearer this-is-the-token", + "this-is-the-token", + true, + }, + { + "Bearer this-is-the-token-with-spaces", + "this-is-the-token-with-spaces", + true, + }, } for _, tc := range tests { @@ -48,8 +51,8 @@ func TestTokenBased(t *testing.T) { handler.ServeHTTP(nil, req) - if mock.defined != tc.defined { - t.Fatalf("Expected defined to be %v but got: %v", tc.defined, mock.defined) + if mock.identityDefined != tc.identityDefined { + t.Fatalf("Expected identityDefined to be %v but got: %v", tc.identityDefined, mock.identityDefined) } if mock.identity != tc.expected {