From dd8ce677ba24ba28fcad26c641e5455f65f5b0b1 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Fri, 14 Aug 2020 10:01:38 -0400 Subject: [PATCH] Remove LoginRequestStatus.User, for now As discussed in API review, this field exists for convenience right now. Since the username/groups are encoded in the Credential sent in the LoginRequestStatus, the client still has access to their user/groups information. We want to remove this for now to be conservative and limit our API surface area (smaller surface area = less to maintain). We can always add this back in the future. Signed-off-by: Andrew Keesler --- internal/registry/loginrequest/rest.go | 4 -- internal/registry/loginrequest/rest_test.go | 4 -- kubernetes/1.19/api/apis/placeholder/types.go | 12 ------ .../api/apis/placeholder/v1alpha1/types.go | 12 ------ .../v1alpha1/zz_generated.conversion.go | 34 --------------- .../v1alpha1/zz_generated.deepcopy.go | 26 ----------- .../apis/placeholder/zz_generated.deepcopy.go | 26 ----------- .../generated/openapi/zz_generated.openapi.go | 43 +------------------ test/integration/loginrequest_test.go | 18 +++++--- 9 files changed, 14 insertions(+), 165 deletions(-) diff --git a/internal/registry/loginrequest/rest.go b/internal/registry/loginrequest/rest.go index 4f66d3df..51809ba5 100644 --- a/internal/registry/loginrequest/rest.go +++ b/internal/registry/loginrequest/rest.go @@ -114,10 +114,6 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation ClientCertificateData: string(certPEM), ClientKeyData: string(keyPEM), }, - User: &placeholderapi.User{ - Name: username, - Groups: groups, - }, }, }, nil } diff --git a/internal/registry/loginrequest/rest_test.go b/internal/registry/loginrequest/rest_test.go index 59e809b4..91bfb353 100644 --- a/internal/registry/loginrequest/rest_test.go +++ b/internal/registry/loginrequest/rest_test.go @@ -114,10 +114,6 @@ func TestCreate(t *testing.T) { r.Equal(response, &placeholderapi.LoginRequest{ Status: placeholderapi.LoginRequestStatus{ - User: &placeholderapi.User{ - Name: "test-user", - Groups: []string{"test-group-1", "test-group-2"}, - }, Credential: &placeholderapi.LoginRequestCredential{ ExpirationTimestamp: metav1.Time{}, ClientCertificateData: "test-cert", diff --git a/kubernetes/1.19/api/apis/placeholder/types.go b/kubernetes/1.19/api/apis/placeholder/types.go index 6253c466..7587af4d 100644 --- a/kubernetes/1.19/api/apis/placeholder/types.go +++ b/kubernetes/1.19/api/apis/placeholder/types.go @@ -40,23 +40,11 @@ type LoginRequestCredential struct { ClientKeyData string } -type User struct { - // Identity Provider name for authenticated user. - Name string - - // Identity Provider groups for authenticated user. - Groups []string -} - type LoginRequestStatus struct { // A Credential will be returned for a successful login request. // +optional Credential *LoginRequestCredential - // A User will be populated from the Identity Provider. - // +optional - User *User - // An error message will be returned for an unsuccessful login request. // +optional Message *string diff --git a/kubernetes/1.19/api/apis/placeholder/v1alpha1/types.go b/kubernetes/1.19/api/apis/placeholder/v1alpha1/types.go index ca8ce922..dcf12c08 100644 --- a/kubernetes/1.19/api/apis/placeholder/v1alpha1/types.go +++ b/kubernetes/1.19/api/apis/placeholder/v1alpha1/types.go @@ -40,23 +40,11 @@ type LoginRequestCredential struct { ClientKeyData string `json:"clientKeyData,omitempty"` } -type User struct { - // Identity Provider name for authenticated user. - Name string `json:"name,omitempty"` - - // Identity Provider groups for authenticated user. - Groups []string `json:"groups"` -} - type LoginRequestStatus struct { // A Credential will be returned for a successful login request. // +optional Credential *LoginRequestCredential `json:"credential,omitempty"` - // A User will be populated from the Identity Provider. - // +optional - User *User `json:"user,omitempty"` - // An error message will be returned for an unsuccessful login request. // +optional Message *string `json:"message,omitempty"` diff --git a/kubernetes/1.19/api/apis/placeholder/v1alpha1/zz_generated.conversion.go b/kubernetes/1.19/api/apis/placeholder/v1alpha1/zz_generated.conversion.go index d8dce5a2..4de5a121 100644 --- a/kubernetes/1.19/api/apis/placeholder/v1alpha1/zz_generated.conversion.go +++ b/kubernetes/1.19/api/apis/placeholder/v1alpha1/zz_generated.conversion.go @@ -84,16 +84,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*User)(nil), (*placeholder.User)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1alpha1_User_To_placeholder_User(a.(*User), b.(*placeholder.User), scope) - }); err != nil { - return err - } - if err := s.AddGeneratedConversionFunc((*placeholder.User)(nil), (*User)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_placeholder_User_To_v1alpha1_User(a.(*placeholder.User), b.(*User), scope) - }); err != nil { - return err - } return nil } @@ -201,7 +191,6 @@ func Convert_placeholder_LoginRequestSpec_To_v1alpha1_LoginRequestSpec(in *place func autoConvert_v1alpha1_LoginRequestStatus_To_placeholder_LoginRequestStatus(in *LoginRequestStatus, out *placeholder.LoginRequestStatus, s conversion.Scope) error { out.Credential = (*placeholder.LoginRequestCredential)(unsafe.Pointer(in.Credential)) - out.User = (*placeholder.User)(unsafe.Pointer(in.User)) out.Message = (*string)(unsafe.Pointer(in.Message)) return nil } @@ -213,7 +202,6 @@ func Convert_v1alpha1_LoginRequestStatus_To_placeholder_LoginRequestStatus(in *L func autoConvert_placeholder_LoginRequestStatus_To_v1alpha1_LoginRequestStatus(in *placeholder.LoginRequestStatus, out *LoginRequestStatus, s conversion.Scope) error { out.Credential = (*LoginRequestCredential)(unsafe.Pointer(in.Credential)) - out.User = (*User)(unsafe.Pointer(in.User)) out.Message = (*string)(unsafe.Pointer(in.Message)) return nil } @@ -242,25 +230,3 @@ func autoConvert_placeholder_LoginRequestTokenCredential_To_v1alpha1_LoginReques func Convert_placeholder_LoginRequestTokenCredential_To_v1alpha1_LoginRequestTokenCredential(in *placeholder.LoginRequestTokenCredential, out *LoginRequestTokenCredential, s conversion.Scope) error { return autoConvert_placeholder_LoginRequestTokenCredential_To_v1alpha1_LoginRequestTokenCredential(in, out, s) } - -func autoConvert_v1alpha1_User_To_placeholder_User(in *User, out *placeholder.User, s conversion.Scope) error { - out.Name = in.Name - out.Groups = *(*[]string)(unsafe.Pointer(&in.Groups)) - return nil -} - -// Convert_v1alpha1_User_To_placeholder_User is an autogenerated conversion function. -func Convert_v1alpha1_User_To_placeholder_User(in *User, out *placeholder.User, s conversion.Scope) error { - return autoConvert_v1alpha1_User_To_placeholder_User(in, out, s) -} - -func autoConvert_placeholder_User_To_v1alpha1_User(in *placeholder.User, out *User, s conversion.Scope) error { - out.Name = in.Name - out.Groups = *(*[]string)(unsafe.Pointer(&in.Groups)) - return nil -} - -// Convert_placeholder_User_To_v1alpha1_User is an autogenerated conversion function. -func Convert_placeholder_User_To_v1alpha1_User(in *placeholder.User, out *User, s conversion.Scope) error { - return autoConvert_placeholder_User_To_v1alpha1_User(in, out, s) -} diff --git a/kubernetes/1.19/api/apis/placeholder/v1alpha1/zz_generated.deepcopy.go b/kubernetes/1.19/api/apis/placeholder/v1alpha1/zz_generated.deepcopy.go index 52ccfcc0..d4c36a2e 100644 --- a/kubernetes/1.19/api/apis/placeholder/v1alpha1/zz_generated.deepcopy.go +++ b/kubernetes/1.19/api/apis/placeholder/v1alpha1/zz_generated.deepcopy.go @@ -120,11 +120,6 @@ func (in *LoginRequestStatus) DeepCopyInto(out *LoginRequestStatus) { *out = new(LoginRequestCredential) (*in).DeepCopyInto(*out) } - if in.User != nil { - in, out := &in.User, &out.User - *out = new(User) - (*in).DeepCopyInto(*out) - } if in.Message != nil { in, out := &in.Message, &out.Message *out = new(string) @@ -158,24 +153,3 @@ func (in *LoginRequestTokenCredential) DeepCopy() *LoginRequestTokenCredential { in.DeepCopyInto(out) return out } - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *User) DeepCopyInto(out *User) { - *out = *in - if in.Groups != nil { - in, out := &in.Groups, &out.Groups - *out = make([]string, len(*in)) - copy(*out, *in) - } - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new User. -func (in *User) DeepCopy() *User { - if in == nil { - return nil - } - out := new(User) - in.DeepCopyInto(out) - return out -} diff --git a/kubernetes/1.19/api/apis/placeholder/zz_generated.deepcopy.go b/kubernetes/1.19/api/apis/placeholder/zz_generated.deepcopy.go index b7fa8f1d..f137b896 100644 --- a/kubernetes/1.19/api/apis/placeholder/zz_generated.deepcopy.go +++ b/kubernetes/1.19/api/apis/placeholder/zz_generated.deepcopy.go @@ -120,11 +120,6 @@ func (in *LoginRequestStatus) DeepCopyInto(out *LoginRequestStatus) { *out = new(LoginRequestCredential) (*in).DeepCopyInto(*out) } - if in.User != nil { - in, out := &in.User, &out.User - *out = new(User) - (*in).DeepCopyInto(*out) - } if in.Message != nil { in, out := &in.Message, &out.Message *out = new(string) @@ -158,24 +153,3 @@ func (in *LoginRequestTokenCredential) DeepCopy() *LoginRequestTokenCredential { in.DeepCopyInto(out) return out } - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *User) DeepCopyInto(out *User) { - *out = *in - if in.Groups != nil { - in, out := &in.Groups, &out.Groups - *out = make([]string, len(*in)) - copy(*out, *in) - } - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new User. -func (in *User) DeepCopy() *User { - if in == nil { - return nil - } - out := new(User) - in.DeepCopyInto(out) - return out -} diff --git a/kubernetes/1.19/api/generated/openapi/zz_generated.openapi.go b/kubernetes/1.19/api/generated/openapi/zz_generated.openapi.go index 09211d30..ea89f2b6 100644 --- a/kubernetes/1.19/api/generated/openapi/zz_generated.openapi.go +++ b/kubernetes/1.19/api/generated/openapi/zz_generated.openapi.go @@ -28,7 +28,6 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "github.com/suzerain-io/placeholder-name/kubernetes/1.19/api/apis/placeholder/v1alpha1.LoginRequestSpec": schema_api_apis_placeholder_v1alpha1_LoginRequestSpec(ref), "github.com/suzerain-io/placeholder-name/kubernetes/1.19/api/apis/placeholder/v1alpha1.LoginRequestStatus": schema_api_apis_placeholder_v1alpha1_LoginRequestStatus(ref), "github.com/suzerain-io/placeholder-name/kubernetes/1.19/api/apis/placeholder/v1alpha1.LoginRequestTokenCredential": schema_api_apis_placeholder_v1alpha1_LoginRequestTokenCredential(ref), - "github.com/suzerain-io/placeholder-name/kubernetes/1.19/api/apis/placeholder/v1alpha1.User": schema_api_apis_placeholder_v1alpha1_User(ref), "k8s.io/apimachinery/pkg/apis/meta/v1.APIGroup": schema_pkg_apis_meta_v1_APIGroup(ref), "k8s.io/apimachinery/pkg/apis/meta/v1.APIGroupList": schema_pkg_apis_meta_v1_APIGroupList(ref), "k8s.io/apimachinery/pkg/apis/meta/v1.APIResource": schema_pkg_apis_meta_v1_APIResource(ref), @@ -365,12 +364,6 @@ func schema_api_apis_placeholder_v1alpha1_LoginRequestStatus(ref common.Referenc Ref: ref("github.com/suzerain-io/placeholder-name/kubernetes/1.19/api/apis/placeholder/v1alpha1.LoginRequestCredential"), }, }, - "user": { - SchemaProps: spec.SchemaProps{ - Description: "A User will be populated from the Identity Provider.", - Ref: ref("github.com/suzerain-io/placeholder-name/kubernetes/1.19/api/apis/placeholder/v1alpha1.User"), - }, - }, "message": { SchemaProps: spec.SchemaProps{ Description: "An error message will be returned for an unsuccessful login request.", @@ -382,7 +375,7 @@ func schema_api_apis_placeholder_v1alpha1_LoginRequestStatus(ref common.Referenc }, }, Dependencies: []string{ - "github.com/suzerain-io/placeholder-name/kubernetes/1.19/api/apis/placeholder/v1alpha1.LoginRequestCredential", "github.com/suzerain-io/placeholder-name/kubernetes/1.19/api/apis/placeholder/v1alpha1.User"}, + "github.com/suzerain-io/placeholder-name/kubernetes/1.19/api/apis/placeholder/v1alpha1.LoginRequestCredential"}, } } @@ -405,40 +398,6 @@ func schema_api_apis_placeholder_v1alpha1_LoginRequestTokenCredential(ref common } } -func schema_api_apis_placeholder_v1alpha1_User(ref common.ReferenceCallback) common.OpenAPIDefinition { - return common.OpenAPIDefinition{ - Schema: spec.Schema{ - SchemaProps: spec.SchemaProps{ - Type: []string{"object"}, - Properties: map[string]spec.Schema{ - "name": { - SchemaProps: spec.SchemaProps{ - Description: "Identity Provider name for authenticated user.", - Type: []string{"string"}, - Format: "", - }, - }, - "groups": { - SchemaProps: spec.SchemaProps{ - Description: "Identity Provider groups for authenticated user.", - Type: []string{"array"}, - Items: &spec.SchemaOrArray{ - Schema: &spec.Schema{ - SchemaProps: spec.SchemaProps{ - Type: []string{"string"}, - Format: "", - }, - }, - }, - }, - }, - }, - Required: []string{"groups"}, - }, - }, - } -} - func schema_pkg_apis_meta_v1_APIGroup(ref common.ReferenceCallback) common.OpenAPIDefinition { return common.OpenAPIDefinition{ Schema: spec.Schema{ diff --git a/test/integration/loginrequest_test.go b/test/integration/loginrequest_test.go index cb46bf63..a0022657 100644 --- a/test/integration/loginrequest_test.go +++ b/test/integration/loginrequest_test.go @@ -7,6 +7,8 @@ package integration import ( "context" + "crypto/x509" + "encoding/pem" "net/http" "testing" "time" @@ -77,9 +79,6 @@ func TestSuccessfulLoginRequest(t *testing.T) { require.NotEmpty(t, response.Status.Credential.ClientKeyData) require.NotNil(t, response.Status.Credential.ExpirationTimestamp) require.InDelta(t, time.Until(response.Status.Credential.ExpirationTimestamp.Time), 1*time.Hour, float64(3*time.Minute)) - require.NotNil(t, response.Status.User) - require.NotEmpty(t, response.Status.User.Name) - require.Contains(t, response.Status.User.Groups, "tmc:member") ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() @@ -103,7 +102,7 @@ func TestSuccessfulLoginRequest(t *testing.T) { Subjects: []rbacv1.Subject{{ Kind: rbacv1.UserKind, APIGroup: rbacv1.GroupName, - Name: response.Status.User.Name, + Name: getCommonName(t, response.Status.Credential.ClientCertificateData), }}, RoleRef: rbacv1.RoleRef{ Kind: "ClusterRole", @@ -154,7 +153,6 @@ func TestFailedLoginRequestWhenTheRequestIsValidButTheTokenDoesNotAuthenticateTh require.Empty(t, response.Spec) require.Nil(t, response.Status.Credential) - require.Nil(t, response.Status.User) require.Equal(t, stringPtr("authentication failed"), response.Status.Message) } @@ -182,3 +180,13 @@ func TestLoginRequest_ShouldFailWhenRequestDoesNotIncludeToken(t *testing.T) { func stringPtr(s string) *string { return &s } + +func getCommonName(t *testing.T, certPEM string) string { + t.Helper() + + pemBlock, _ := pem.Decode([]byte(certPEM)) + cert, err := x509.ParseCertificate(pemBlock.Bytes) + require.NoError(t, err) + + return cert.Subject.CommonName +}