Skip to content

Commit

Permalink
TokenCredentialRequest should ignore extra key authentication.kuberne…
Browse files Browse the repository at this point in the history
…tes.io/credential-id
  • Loading branch information
joshuatcasey committed Dec 18, 2024
1 parent 0562e68 commit 2aea1c5
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 8 deletions.
28 changes: 21 additions & 7 deletions internal/registry/credentialrequest/rest.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,16 +240,30 @@ func validateRequest(ctx context.Context, obj runtime.Object, createValidation r
}

func validateUserInfo(userInfo user.Info) error {
switch {
case len(userInfo.GetName()) == 0:
if len(userInfo.GetName()) == 0 {
return errors.New("empty username is not allowed")
case len(userInfo.GetUID()) != 0:
return errors.New("UIDs are not supported") // certs cannot assert UID
case len(userInfo.GetExtra()) != 0:
return errors.New("extras are not supported") // certs cannot assert extra
default:
}

// certs cannot assert UID
if len(userInfo.GetUID()) != 0 {
return errors.New("UIDs are not supported")
}

// certs cannot assert extras, but starting in K8s 1.32 the authenticator will always provide this information
if len(userInfo.GetExtra()) == 0 { // it's ok for this to be empty...
return nil
}

// ... but if it's not empty, should have only exactly this one key.
if len(userInfo.GetExtra()) > 1 {
return errors.New("extra may have only one key 'authentication.kubernetes.io/credential-id'")
}

_, ok := userInfo.GetExtra()["authentication.kubernetes.io/credential-id"]
if !ok {
return errors.New("extra may have only one key 'authentication.kubernetes.io/credential-id'")
}
return nil
}

func authenticationFailedResponse() *loginapi.TokenCredentialRequest {
Expand Down
109 changes: 108 additions & 1 deletion internal/registry/credentialrequest/rest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ func TestCreate(t *testing.T) {
"name": "fake-authenticator-name",
},
"reason": "unsupported value in userInfo returned by authenticator",
"err": "extras are not supported",
"err": "extra may have only one key 'authentication.kubernetes.io/credential-id'",
"userInfoExtrasCount": float64(1),
"personalInfo": map[string]any{
"userInfoName": "test-user",
Expand All @@ -369,6 +369,113 @@ func TestCreate(t *testing.T) {
}
})

it("CreateSucceedsWithAnUnauthenticatedStatusWhenWebhookReturnsAUserWithTooManyExtra", func() {
req := validCredentialRequest()

requestAuthenticator := mockcredentialrequest.NewMockTokenCredentialRequestAuthenticator(ctrl)
requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req).
Return(&user.DefaultInfo{
Name: "test-user",
Groups: []string{"test-group-1", "test-group-2"},
Extra: map[string][]string{
"test-key": {"test-val-1", "test-val-2"},
"authentication.kubernetes.io/credential-id": {"some-value"},
},
}, nil)

storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}, auditLogger)

response, err := callCreate(storage, req)

requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response)

wantAuditLog = []testutil.WantedAuditLog{
testutil.WantAuditLog("TokenCredentialRequest Token Received", map[string]any{
"auditID": "fake-audit-id",
"tokenID": tokenToHash(req.Spec.Token),
}),
testutil.WantAuditLog("TokenCredentialRequest Unsupported UserInfo", map[string]any{
"auditID": "fake-audit-id",
"authenticator": map[string]any{
"apiGroup": "fake-api-group.com",
"kind": "FakeAuthenticatorKind",
"name": "fake-authenticator-name",
},
"reason": "unsupported value in userInfo returned by authenticator",
"err": "extra may have only one key 'authentication.kubernetes.io/credential-id'",
"userInfoExtrasCount": float64(2),
"personalInfo": map[string]any{
"userInfoName": "test-user",
"userInfoUID": "",
},
}),
}
})

it("CreateSucceedsWhenWebhookReturnsAUserWithValidExtra", func() {
req := validCredentialRequest()

requestAuthenticator := mockcredentialrequest.NewMockTokenCredentialRequestAuthenticator(ctrl)
requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req).
Return(&user.DefaultInfo{
Name: "test-user",
Groups: []string{"test-group-1", "test-group-2"},
Extra: map[string][]string{"authentication.kubernetes.io/credential-id": {"test-val-1", "test-val-2"}},
}, nil)

clientCertIssuer := mockissuer.NewMockClientCertIssuer(ctrl)
clientCertIssuer.EXPECT().IssueClientCertPEM(
"test-user",
[]string{"test-group-1", "test-group-2"},
5*time.Minute,
).Return(&cert.PEM{
CertPEM: []byte("test-cert"),
KeyPEM: []byte("test-key"),
NotBefore: fakeNow.Add(-5 * time.Minute),
NotAfter: fakeNow.Add(5 * time.Minute),
}, nil)

storage := NewREST(requestAuthenticator, clientCertIssuer, schema.GroupResource{}, auditLogger)

response, err := callCreate(storage, req)

r.NoError(err)
r.IsType(&loginapi.TokenCredentialRequest{}, response)

r.Equal(response, &loginapi.TokenCredentialRequest{
Status: loginapi.TokenCredentialRequestStatus{
Credential: &loginapi.ClusterCredential{
ExpirationTimestamp: metav1.NewTime(fakeNow.Add(5 * time.Minute).UTC()),
ClientCertificateData: "test-cert",
ClientKeyData: "test-key",
},
},
})

wantAuditLog = []testutil.WantedAuditLog{
testutil.WantAuditLog("TokenCredentialRequest Token Received", map[string]any{
"auditID": "fake-audit-id",
"tokenID": tokenToHash(req.Spec.Token),
}),
testutil.WantAuditLog("TokenCredentialRequest Authenticated User", map[string]any{
"auditID": "fake-audit-id",
"authenticator": map[string]any{
"apiGroup": "fake-api-group.com",
"kind": "FakeAuthenticatorKind",
"name": "fake-authenticator-name",
},
"issuedClientCert": map[string]any{
"notBefore": "2024-09-12T04:20:56Z", // this is fakeNow - 5 minutes in UTC
"notAfter": "2024-09-12T04:30:56Z", // this is fakeNow + 5 minutes in UTC
},
"personalInfo": map[string]any{
"username": "test-user",
"groups": []any{"test-group-1", "test-group-2"},
},
}),
}
})

it("CreateFailsWhenGivenTheWrongInputType", func() {
notACredentialRequest := runtime.Unknown{}
response, err := NewREST(nil, nil, schema.GroupResource{}, auditLogger).Create(
Expand Down
1 change: 1 addition & 0 deletions test/integration/formposthtml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ func formpostExpectSuccessState(t *testing.T, b *browsertest.Browser) {
require.Contains(t, successDivText, "Login succeeded")
require.Contains(t, successDivText, "You have successfully logged in. You may now close this tab.")
require.Equal(t, "Login succeeded", b.Title(t))
t.Log("login succeeded")
formpostExpectFavicon(t, b, "✅")
}

Expand Down

0 comments on commit 2aea1c5

Please sign in to comment.