From 2ad2bd8907d979f709cd924af7986be71c31ce12 Mon Sep 17 00:00:00 2001 From: Taahir Ahmed Date: Fri, 21 Jun 2024 16:21:35 -0700 Subject: [PATCH] Define credential IDs for X.509 certificates This commit expands the existing credential ID concept to cover X.509 certificates. We use the certificate's signature as the credential ID, since this safe and unique. --- .../pkg/authentication/request/x509/x509.go | 8 ++ .../authentication/request/x509/x509_test.go | 133 +++++++++++------- .../pkg/authentication/serviceaccount/util.go | 5 +- .../apiserver/pkg/authentication/user/user.go | 6 +- test/integration/auth/svcaccttoken_test.go | 7 +- 5 files changed, 97 insertions(+), 62 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509.go b/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509.go index d67c5354763..bdc1b1790e3 100644 --- a/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509.go +++ b/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509.go @@ -17,6 +17,7 @@ limitations under the License. package x509 import ( + "crypto/sha256" "crypto/x509" "crypto/x509/pkix" "encoding/hex" @@ -276,10 +277,17 @@ var CommonNameUserConversion = UserConversionFunc(func(chain []*x509.Certificate if len(chain[0].Subject.CommonName) == 0 { return nil, false, nil } + + fp := sha256.Sum256(chain[0].Raw) + id := "X509SHA256=" + hex.EncodeToString(fp[:]) + return &authenticator.Response{ User: &user.DefaultInfo{ Name: chain[0].Subject.CommonName, Groups: chain[0].Subject.Organization, + Extra: map[string][]string{ + user.CredentialIDKey: {id}, + }, }, }, true, nil }) diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509_test.go b/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509_test.go index efb6f2aca84..31a78112223 100644 --- a/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509_test.go +++ b/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509_test.go @@ -23,11 +23,11 @@ import ( "errors" "io/ioutil" "net/http" - "reflect" "sort" "testing" "time" + "github.com/google/go-cmp/cmp" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" @@ -581,9 +581,8 @@ func TestX509(t *testing.T) { Opts x509.VerifyOptions User UserConversion - ExpectUserName string - ExpectGroups []string ExpectOK bool + ExpectResponse *authenticator.Response ExpectErr bool }{ "non-tls": { @@ -618,10 +617,17 @@ func TestX509(t *testing.T) { Certs: getCerts(t, serverCert), User: CommonNameUserConversion, - ExpectUserName: "127.0.0.1", - ExpectGroups: []string{"My Org"}, - ExpectOK: true, - ExpectErr: false, + ExpectOK: true, + ExpectResponse: &authenticator.Response{ + User: &user.DefaultInfo{ + Name: "127.0.0.1", + Groups: []string{"My Org"}, + Extra: map[string][]string{ + user.CredentialIDKey: {"X509SHA256=92209d1e0dd36a018f244f5e1b88e2d47b049e9cfcd4b7c87c65875866872230"}, + }, + }, + }, + ExpectErr: false, }, "common name": { @@ -629,10 +635,17 @@ func TestX509(t *testing.T) { Certs: getCerts(t, clientCNCert), User: CommonNameUserConversion, - ExpectUserName: "client_cn", - ExpectGroups: []string{"My Org"}, - ExpectOK: true, - ExpectErr: false, + ExpectOK: true, + ExpectResponse: &authenticator.Response{ + User: &user.DefaultInfo{ + Name: "client_cn", + Groups: []string{"My Org"}, + Extra: map[string][]string{ + user.CredentialIDKey: {"X509SHA256=dd0a6a295055fa94455c522b0d54ef0499186f454a7cf978b8b346dc35b254f7"}, + }, + }, + }, + ExpectErr: false, }, "ca with multiple organizations": { Opts: x509.VerifyOptions{ @@ -641,10 +654,17 @@ func TestX509(t *testing.T) { Certs: getCerts(t, caWithGroups), User: CommonNameUserConversion, - ExpectUserName: "ROOT CA WITH GROUPS", - ExpectGroups: []string{"My Org", "My Org 1", "My Org 2"}, - ExpectOK: true, - ExpectErr: false, + ExpectOK: true, + ExpectResponse: &authenticator.Response{ + User: &user.DefaultInfo{ + Name: "ROOT CA WITH GROUPS", + Groups: []string{"My Org", "My Org 1", "My Org 2"}, + Extra: map[string][]string{ + user.CredentialIDKey: {"X509SHA256=6f337bb6576b6f942bd5ac5256f621e352aa7b34d971bda9b8f8981f51bba456"}, + }, + }, + }, + ExpectErr: false, }, "custom conversion error": { @@ -664,9 +684,13 @@ func TestX509(t *testing.T) { return &authenticator.Response{User: &user.DefaultInfo{Name: "custom"}}, true, nil }), - ExpectUserName: "custom", - ExpectOK: true, - ExpectErr: false, + ExpectOK: true, + ExpectResponse: &authenticator.Response{ + User: &user.DefaultInfo{ + Name: "custom", + }, + }, + ExpectErr: false, }, "future cert": { @@ -697,9 +721,16 @@ func TestX509(t *testing.T) { Certs: getCertsFromFile(t, "client-valid", "intermediate"), User: CommonNameUserConversion, - ExpectUserName: "My Client", - ExpectOK: true, - ExpectErr: false, + ExpectOK: true, + ExpectResponse: &authenticator.Response{ + User: &user.DefaultInfo{ + Name: "My Client", + Extra: map[string][]string{ + user.CredentialIDKey: {"X509SHA256=794b0529fd1a72d55d52d98be9bab5b822d16f9ae86c4373fa7beee3cafe8582"}, + }, + }, + }, + ExpectErr: false, }, "multi-level, expired": { Opts: multilevelOpts, @@ -712,42 +743,36 @@ func TestX509(t *testing.T) { } for k, testCase := range testCases { - req, _ := http.NewRequest("GET", "/", nil) - if !testCase.Insecure { - req.TLS = &tls.ConnectionState{PeerCertificates: testCase.Certs} - } - - // this effectively tests the simple dynamic verify function. - a := New(testCase.Opts, testCase.User) - - resp, ok, err := a.AuthenticateRequest(req) - - if testCase.ExpectErr && err == nil { - t.Errorf("%s: Expected error, got none", k) - continue - } - if !testCase.ExpectErr && err != nil { - t.Errorf("%s: Got unexpected error: %v", k, err) - continue - } - - if testCase.ExpectOK != ok { - t.Errorf("%s: Expected ok=%v, got %v", k, testCase.ExpectOK, ok) - continue - } - - if testCase.ExpectOK { - if testCase.ExpectUserName != resp.User.GetName() { - t.Errorf("%s: Expected user.name=%v, got %v", k, testCase.ExpectUserName, resp.User.GetName()) + t.Run(k, func(t *testing.T) { + req, _ := http.NewRequest("GET", "/", nil) + if !testCase.Insecure { + req.TLS = &tls.ConnectionState{PeerCertificates: testCase.Certs} } - groups := resp.User.GetGroups() - sort.Strings(testCase.ExpectGroups) - sort.Strings(groups) - if !reflect.DeepEqual(testCase.ExpectGroups, groups) { - t.Errorf("%s: Expected user.groups=%v, got %v", k, testCase.ExpectGroups, groups) + // this effectively tests the simple dynamic verify function. + a := New(testCase.Opts, testCase.User) + + resp, ok, err := a.AuthenticateRequest(req) + + if testCase.ExpectErr && err == nil { + t.Fatalf("Expected error, got none") } - } + if !testCase.ExpectErr && err != nil { + t.Fatalf("Got unexpected error: %v", err) + } + + if testCase.ExpectOK != ok { + t.Fatalf("Expected ok=%v, got %v", testCase.ExpectOK, ok) + } + + if testCase.ExpectOK { + sort.Strings(testCase.ExpectResponse.User.GetGroups()) + sort.Strings(resp.User.GetGroups()) + if diff := cmp.Diff(testCase.ExpectResponse, resp); diff != "" { + t.Errorf("Bad response; diff (-want +got)\n%s", diff) + } + } + }) } } diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/serviceaccount/util.go b/staging/src/k8s.io/apiserver/pkg/authentication/serviceaccount/util.go index 3e29d4e71c9..93dbefae33e 100644 --- a/staging/src/k8s.io/apiserver/pkg/authentication/serviceaccount/util.go +++ b/staging/src/k8s.io/apiserver/pkg/authentication/serviceaccount/util.go @@ -36,9 +36,6 @@ const ( ServiceAccountUsernameSeparator = ":" ServiceAccountGroupPrefix = "system:serviceaccounts:" AllServiceAccountsGroup = "system:serviceaccounts" - // CredentialIDKey is the key used in a user's "extra" to specify the unique - // identifier for this identity document). - CredentialIDKey = "authentication.kubernetes.io/credential-id" // IssuedCredentialIDAuditAnnotationKey is the annotation key used in the audit event that is persisted to the // '/token' endpoint for service accounts. // This annotation indicates the generated credential identifier for the service account token being issued. @@ -156,7 +153,7 @@ func (sa *ServiceAccountInfo) UserInfo() user.Info { if info.Extra == nil { info.Extra = make(map[string][]string) } - info.Extra[CredentialIDKey] = []string{sa.CredentialID} + info.Extra[user.CredentialIDKey] = []string{sa.CredentialID} } if sa.NodeName != "" { if info.Extra == nil { diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/user/user.go b/staging/src/k8s.io/apiserver/pkg/authentication/user/user.go index 4d6ec098002..1af6f2b2774 100644 --- a/staging/src/k8s.io/apiserver/pkg/authentication/user/user.go +++ b/staging/src/k8s.io/apiserver/pkg/authentication/user/user.go @@ -66,8 +66,8 @@ func (i *DefaultInfo) GetExtra() map[string][]string { return i.Extra } -// well-known user and group names const ( + // well-known user and group names SystemPrivilegedGroup = "system:masters" NodesGroup = "system:nodes" MonitoringGroup = "system:monitoring" @@ -81,4 +81,8 @@ const ( KubeProxy = "system:kube-proxy" KubeControllerManager = "system:kube-controller-manager" KubeScheduler = "system:kube-scheduler" + + // CredentialIDKey is the key used in a user's "extra" to specify the unique + // identifier for this identity document). + CredentialIDKey = "authentication.kubernetes.io/credential-id" ) diff --git a/test/integration/auth/svcaccttoken_test.go b/test/integration/auth/svcaccttoken_test.go index c88e35a144e..f05a9dfe34a 100644 --- a/test/integration/auth/svcaccttoken_test.go +++ b/test/integration/auth/svcaccttoken_test.go @@ -41,6 +41,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apiserver/pkg/authentication/authenticator" apiserverserviceaccount "k8s.io/apiserver/pkg/authentication/serviceaccount" + "k8s.io/apiserver/pkg/authentication/user" utilfeature "k8s.io/apiserver/pkg/util/feature" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" @@ -237,7 +238,7 @@ func TestServiceAccountTokenCreate(t *testing.T) { info := doTokenReview(t, cs, treq, false) // we are not testing the credential-id feature, so delete this value from the returned extra info map if info.Extra != nil { - delete(info.Extra, apiserverserviceaccount.CredentialIDKey) + delete(info.Extra, user.CredentialIDKey) } if len(info.Extra) > 0 { t.Fatalf("expected Extra to be empty but got: %#v", info.Extra) @@ -309,7 +310,7 @@ func TestServiceAccountTokenCreate(t *testing.T) { info := doTokenReview(t, cs, treq, false) // we are not testing the credential-id feature, so delete this value from the returned extra info map - delete(info.Extra, apiserverserviceaccount.CredentialIDKey) + delete(info.Extra, user.CredentialIDKey) if len(info.Extra) != 2 { t.Fatalf("expected Extra have length of 2 but was length %d: %#v", len(info.Extra), info.Extra) } @@ -405,7 +406,7 @@ func TestServiceAccountTokenCreate(t *testing.T) { info := doTokenReview(t, cs, treq, false) // we are not testing the credential-id feature, so delete this value from the returned extra info map - delete(info.Extra, apiserverserviceaccount.CredentialIDKey) + delete(info.Extra, user.CredentialIDKey) if len(info.Extra) != len(expectedExtraValues) { t.Fatalf("expected Extra have length of %d but was length %d: %#v", len(expectedExtraValues), len(info.Extra), info.Extra) }