From 6c644aeceb7d3904d0e59281bc45a3fd6c85d65e Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Mon, 25 Sep 2023 11:30:49 +0100 Subject: [PATCH 1/3] Add support for Protobuf Enums into Operator CRDs This PR marks the Teleport enum fields as integer or string values. The integer option is to ensure we are backwards compatibile with previously installed CRDs. Users can now represent their roles in Kubernetes custom resources and refer enum fields as strings while their protobuf wire type is int32. Fixes #29686 --- api/types/authentication.go | 19 +++++ api/types/extension.go | 72 +++++++++++++++---- api/types/role.go | 21 +++++- api/types/role_test.go | 14 ++-- .../resources.teleport.dev_roles.yaml | 24 +++---- .../bases/resources.teleport.dev_roles.yaml | 24 +++---- integrations/operator/crdgen/schemagen.go | 4 +- 7 files changed, 126 insertions(+), 52 deletions(-) diff --git a/api/types/authentication.go b/api/types/authentication.go index a074bb12c6129..fb6376d13638b 100644 --- a/api/types/authentication.go +++ b/api/types/authentication.go @@ -941,8 +941,27 @@ func (r *RequireMFAType) decode(val interface{}) error { } else { *r = RequireMFAType_OFF } + case int32: + return trace.Wrap(r.setFromEnum(v)) + case int64: + return trace.Wrap(r.setFromEnum(int32(v))) + case int: + return trace.Wrap(r.setFromEnum(int32(v))) + case float64: + return trace.Wrap(r.setFromEnum(int32(v))) + case float32: + return trace.Wrap(r.setFromEnum(int32(v))) default: return trace.BadParameter("RequireMFAType invalid type %T", val) } return nil } + +// setFromEnum sets the value from enum value as int32. +func (r *RequireMFAType) setFromEnum(val int32) error { + if _, ok := RequireMFAType_name[val]; !ok { + return trace.BadParameter("invalid required mfa mode %v", val) + } + *r = RequireMFAType(val) + return nil +} diff --git a/api/types/extension.go b/api/types/extension.go index e44cc4a95f02b..fcfb5ef78a74a 100644 --- a/api/types/extension.go +++ b/api/types/extension.go @@ -39,16 +39,40 @@ func (t CertExtensionType) MarshalJSON() ([]byte, error) { } func (t *CertExtensionType) UnmarshalJSON(b []byte) error { - var stringVal string - if err := json.Unmarshal(b, &stringVal); err != nil { + var anyVal any + if err := json.Unmarshal(b, &anyVal); err != nil { return err } - val, ok := certExtensionTypeValue[stringVal] - if !ok { - return trace.Errorf("invalid certificate extension type: %q", string(b)) + switch val := anyVal.(type) { + case string: + enumVal, ok := certExtensionTypeValue[val] + if !ok { + return trace.Errorf("invalid certificate extension type: %q", string(b)) + } + *t = enumVal + return nil + case int32: + return t.setFromEnum(val) + case int: + return t.setFromEnum(int32(val)) + case int64: + return t.setFromEnum(int32(val)) + case float64: + return trace.Wrap(t.setFromEnum(int32(val))) + case float32: + return trace.Wrap(t.setFromEnum(int32(val))) + default: + return trace.BadParameter("unexpected type %T", val) + } +} + +// setFromEnum sets the value from enum value as int32. +func (t *CertExtensionType) setFromEnum(val int32) error { + if _, ok := CertExtensionType_name[val]; !ok { + return trace.BadParameter("invalid cert extension mode %v", val) } - *t = val + *t = CertExtensionType(val) return nil } @@ -69,14 +93,38 @@ func (t CertExtensionMode) MarshalJSON() ([]byte, error) { } func (t *CertExtensionMode) UnmarshalJSON(b []byte) error { - var stringVal string - if err := json.Unmarshal(b, &stringVal); err != nil { + var anyVal any + if err := json.Unmarshal(b, &anyVal); err != nil { return err } - val, ok := certExtensionModeValue[stringVal] - if !ok { - return trace.Errorf("invalid certificate extension mode: %q", string(b)) + switch val := anyVal.(type) { + case string: + enumVal, ok := certExtensionModeValue[val] + if !ok { + return trace.Errorf("invalid certificate extension mode: %q", string(b)) + } + *t = enumVal + return nil + case int32: + return t.setFromEnum(val) + case int: + return t.setFromEnum(int32(val)) + case int64: + return t.setFromEnum(int32(val)) + case float64: + return trace.Wrap(t.setFromEnum(int32(val))) + case float32: + return trace.Wrap(t.setFromEnum(int32(val))) + default: + return trace.BadParameter("unexpected type %T", val) + } +} + +// setFromEnum sets the value from enum value as int32. +func (t *CertExtensionMode) setFromEnum(val int32) error { + if _, ok := CertExtensionMode_name[val]; !ok { + return trace.BadParameter("invalid cert extension mode %v", val) } - *t = val + *t = CertExtensionMode(val) return nil } diff --git a/api/types/role.go b/api/types/role.go index 9389ecdd0f7db..adbfa46f36b8f 100644 --- a/api/types/role.go +++ b/api/types/role.go @@ -1698,6 +1698,16 @@ func (h CreateHostUserMode) encode() (string, error) { func (h *CreateHostUserMode) decode(val any) error { var valS string switch val := val.(type) { + case int32: + return trace.Wrap(h.setFromEnum(val)) + case int64: + return trace.Wrap(h.setFromEnum(int32(val))) + case int: + return trace.Wrap(h.setFromEnum(int32(val))) + case float64: + return trace.Wrap(h.setFromEnum(int32(val))) + case float32: + return trace.Wrap(h.setFromEnum(int32(val))) case string: valS = val case bool: @@ -1706,7 +1716,7 @@ func (h *CreateHostUserMode) decode(val any) error { } valS = createHostUserModeOffString default: - return trace.BadParameter("bad value type %T, expected string", val) + return trace.BadParameter("bad value type %T, expected string or int", val) } switch valS { @@ -1724,6 +1734,15 @@ func (h *CreateHostUserMode) decode(val any) error { return nil } +// setFromEnum sets the value from enum value as int32. +func (h *CreateHostUserMode) setFromEnum(val int32) error { + if _, ok := CreateHostUserMode_name[val]; !ok { + return trace.BadParameter("invalid host user mode %v", val) + } + *h = CreateHostUserMode(val) + return nil +} + // UnmarshalYAML supports parsing CreateHostUserMode from string. func (h *CreateHostUserMode) UnmarshalYAML(unmarshal func(interface{}) error) error { var val interface{} diff --git a/api/types/role_test.go b/api/types/role_test.go index 140e0b68b6734..53f7bd53f8704 100644 --- a/api/types/role_test.go +++ b/api/types/role_test.go @@ -184,15 +184,17 @@ func TestMarshallCreateHostUserModeYAML(t *testing.T) { func TestUnmarshallCreateHostUserModeJSON(t *testing.T) { for _, tc := range []struct { expected CreateHostUserMode - input string + input any }{ - {expected: CreateHostUserMode_HOST_USER_MODE_OFF, input: "off"}, - {expected: CreateHostUserMode_HOST_USER_MODE_UNSPECIFIED, input: ""}, - {expected: CreateHostUserMode_HOST_USER_MODE_DROP, input: "drop"}, - {expected: CreateHostUserMode_HOST_USER_MODE_KEEP, input: "keep"}, + {expected: CreateHostUserMode_HOST_USER_MODE_OFF, input: "\"off\""}, + {expected: CreateHostUserMode_HOST_USER_MODE_UNSPECIFIED, input: "\"\""}, + {expected: CreateHostUserMode_HOST_USER_MODE_DROP, input: "\"drop\""}, + {expected: CreateHostUserMode_HOST_USER_MODE_KEEP, input: "\"keep\""}, + {expected: CreateHostUserMode_HOST_USER_MODE_KEEP, input: 3}, + {expected: CreateHostUserMode_HOST_USER_MODE_OFF, input: 1}, } { var got CreateHostUserMode - err := json.Unmarshal([]byte(fmt.Sprintf("%q", tc.input)), &got) + err := json.Unmarshal([]byte(fmt.Sprintf("%v", tc.input)), &got) require.NoError(t, err) require.Equal(t, tc.expected, got) } diff --git a/examples/chart/teleport-cluster/charts/teleport-operator/templates/resources.teleport.dev_roles.yaml b/examples/chart/teleport-cluster/charts/teleport-operator/templates/resources.teleport.dev_roles.yaml index b305702e52ccb..8883913816d4a 100644 --- a/examples/chart/teleport-cluster/charts/teleport-operator/templates/resources.teleport.dev_roles.yaml +++ b/examples/chart/teleport-cluster/charts/teleport-operator/templates/resources.teleport.dev_roles.yaml @@ -949,8 +949,7 @@ spec: mode: description: Mode is the type of extension to be used -- currently critical-option is not supported - format: int32 - type: integer + x-kubernetes-int-or-string: true name: description: Name specifies the key to be used in the cert extension. @@ -958,8 +957,7 @@ spec: type: description: Type represents the certificate type being extended, only ssh is supported at this time. - format: int32 - type: integer + x-kubernetes-int-or-string: true value: description: Value specifies the value to be used in the cert extension. @@ -992,8 +990,7 @@ spec: create_host_user_mode: description: CreateHostUserMode allows users to be automatically created on a host when not set to off - format: int32 - type: integer + x-kubernetes-int-or-string: true desktop_clipboard: description: DesktopClipboard indicates whether clipboard sharing is allowed between the user's workstation and the remote desktop. @@ -1104,8 +1101,7 @@ spec: require_session_mfa: description: RequireMFAType is the type of MFA requirement enforced for this user. - format: int32 - type: integer + x-kubernetes-int-or-string: true ssh_file_copy: description: SSHFileCopy indicates whether remote file operations via SCP or SFTP are allowed over an SSH session. It defaults @@ -2132,8 +2128,7 @@ spec: mode: description: Mode is the type of extension to be used -- currently critical-option is not supported - format: int32 - type: integer + x-kubernetes-int-or-string: true name: description: Name specifies the key to be used in the cert extension. @@ -2141,8 +2136,7 @@ spec: type: description: Type represents the certificate type being extended, only ssh is supported at this time. - format: int32 - type: integer + x-kubernetes-int-or-string: true value: description: Value specifies the value to be used in the cert extension. @@ -2175,8 +2169,7 @@ spec: create_host_user_mode: description: CreateHostUserMode allows users to be automatically created on a host when not set to off - format: int32 - type: integer + x-kubernetes-int-or-string: true desktop_clipboard: description: DesktopClipboard indicates whether clipboard sharing is allowed between the user's workstation and the remote desktop. @@ -2287,8 +2280,7 @@ spec: require_session_mfa: description: RequireMFAType is the type of MFA requirement enforced for this user. - format: int32 - type: integer + x-kubernetes-int-or-string: true ssh_file_copy: description: SSHFileCopy indicates whether remote file operations via SCP or SFTP are allowed over an SSH session. It defaults diff --git a/integrations/operator/config/crd/bases/resources.teleport.dev_roles.yaml b/integrations/operator/config/crd/bases/resources.teleport.dev_roles.yaml index d8bb0486308fb..6398238e12c49 100644 --- a/integrations/operator/config/crd/bases/resources.teleport.dev_roles.yaml +++ b/integrations/operator/config/crd/bases/resources.teleport.dev_roles.yaml @@ -949,8 +949,7 @@ spec: mode: description: Mode is the type of extension to be used -- currently critical-option is not supported - format: int32 - type: integer + x-kubernetes-int-or-string: true name: description: Name specifies the key to be used in the cert extension. @@ -958,8 +957,7 @@ spec: type: description: Type represents the certificate type being extended, only ssh is supported at this time. - format: int32 - type: integer + x-kubernetes-int-or-string: true value: description: Value specifies the value to be used in the cert extension. @@ -992,8 +990,7 @@ spec: create_host_user_mode: description: CreateHostUserMode allows users to be automatically created on a host when not set to off - format: int32 - type: integer + x-kubernetes-int-or-string: true desktop_clipboard: description: DesktopClipboard indicates whether clipboard sharing is allowed between the user's workstation and the remote desktop. @@ -1104,8 +1101,7 @@ spec: require_session_mfa: description: RequireMFAType is the type of MFA requirement enforced for this user. - format: int32 - type: integer + x-kubernetes-int-or-string: true ssh_file_copy: description: SSHFileCopy indicates whether remote file operations via SCP or SFTP are allowed over an SSH session. It defaults @@ -2132,8 +2128,7 @@ spec: mode: description: Mode is the type of extension to be used -- currently critical-option is not supported - format: int32 - type: integer + x-kubernetes-int-or-string: true name: description: Name specifies the key to be used in the cert extension. @@ -2141,8 +2136,7 @@ spec: type: description: Type represents the certificate type being extended, only ssh is supported at this time. - format: int32 - type: integer + x-kubernetes-int-or-string: true value: description: Value specifies the value to be used in the cert extension. @@ -2175,8 +2169,7 @@ spec: create_host_user_mode: description: CreateHostUserMode allows users to be automatically created on a host when not set to off - format: int32 - type: integer + x-kubernetes-int-or-string: true desktop_clipboard: description: DesktopClipboard indicates whether clipboard sharing is allowed between the user's workstation and the remote desktop. @@ -2287,8 +2280,7 @@ spec: require_session_mfa: description: RequireMFAType is the type of MFA requirement enforced for this user. - format: int32 - type: integer + x-kubernetes-int-or-string: true ssh_file_copy: description: SSHFileCopy indicates whether remote file operations via SCP or SFTP are allowed over an SSH session. It defaults diff --git a/integrations/operator/crdgen/schemagen.go b/integrations/operator/crdgen/schemagen.go index d461da84f9556..ed633faf25ec8 100644 --- a/integrations/operator/crdgen/schemagen.go +++ b/integrations/operator/crdgen/schemagen.go @@ -297,9 +297,11 @@ func (generator *SchemaGenerator) singularProp(field *Field, prop *apiextv1.JSON case field.IsTime(): prop.Type = "string" prop.Format = "date-time" - case field.IsInt32() || field.IsUint32() || field.desc.IsEnum(): + case field.IsInt32() || field.IsUint32(): prop.Type = "integer" prop.Format = "int32" + case field.desc.IsEnum(): + prop.XIntOrString = true case field.IsInt64() || field.IsUint64(): prop.Type = "integer" prop.Format = "int64" From 6c117d80b7c49ab40d777aff86a04955265198e5 Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Mon, 25 Sep 2023 13:18:46 +0100 Subject: [PATCH 2/3] add tests --- .../resources/role_controller_test.go | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/integrations/operator/controllers/resources/role_controller_test.go b/integrations/operator/controllers/resources/role_controller_test.go index d5744415c9018..ceff5fdde7e31 100644 --- a/integrations/operator/controllers/resources/role_controller_test.go +++ b/integrations/operator/controllers/resources/role_controller_test.go @@ -82,6 +82,26 @@ func TestRoleCreationFromYAML(t *testing.T) { shouldFail bool expectedSpec *types.RoleSpecV6 }{ + { + name: "Valid login list with integer create_host_user_mode", + roleSpecYAML: ` +allow: + logins: + - ubuntu + - root +options: + create_host_user_mode: 2 +`, + shouldFail: false, + expectedSpec: &types.RoleSpecV6{ + Allow: types.RoleConditions{ + Logins: []string{"ubuntu", "root"}, + }, + Options: types.RoleOptions{ + CreateHostUserMode: types.CreateHostUserMode_HOST_USER_MODE_DROP, + }, + }, + }, { name: "Valid login list", roleSpecYAML: ` @@ -89,12 +109,17 @@ allow: logins: - ubuntu - root +options: + create_host_user_mode: "keep" `, shouldFail: false, expectedSpec: &types.RoleSpecV6{ Allow: types.RoleConditions{ Logins: []string{"ubuntu", "root"}, }, + Options: types.RoleOptions{ + CreateHostUserMode: types.CreateHostUserMode_HOST_USER_MODE_KEEP, + }, }, }, { From d71d818f19324738ed06cd429105a4efe5239d81 Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Mon, 25 Sep 2023 15:51:00 +0100 Subject: [PATCH 3/3] fix unit test --- .../resources/role_controller_test.go | 4 ++-- .../golden/resources.teleport.dev_roles.yaml | 24 +++++++------------ 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/integrations/operator/controllers/resources/role_controller_test.go b/integrations/operator/controllers/resources/role_controller_test.go index ceff5fdde7e31..34f739a8a3721 100644 --- a/integrations/operator/controllers/resources/role_controller_test.go +++ b/integrations/operator/controllers/resources/role_controller_test.go @@ -90,7 +90,7 @@ allow: - ubuntu - root options: - create_host_user_mode: 2 + create_host_user_mode: 2 `, shouldFail: false, expectedSpec: &types.RoleSpecV6{ @@ -110,7 +110,7 @@ allow: - ubuntu - root options: - create_host_user_mode: "keep" + create_host_user_mode: keep `, shouldFail: false, expectedSpec: &types.RoleSpecV6{ diff --git a/integrations/operator/crdgen/testdata/golden/resources.teleport.dev_roles.yaml b/integrations/operator/crdgen/testdata/golden/resources.teleport.dev_roles.yaml index f88328a2e3ea9..ac49f4b375dad 100644 --- a/integrations/operator/crdgen/testdata/golden/resources.teleport.dev_roles.yaml +++ b/integrations/operator/crdgen/testdata/golden/resources.teleport.dev_roles.yaml @@ -963,8 +963,7 @@ spec: mode: description: Mode is the type of extension to be used -- currently critical-option is not supported - format: int32 - type: integer + x-kubernetes-int-or-string: true name: description: Name specifies the key to be used in the cert extension. @@ -972,8 +971,7 @@ spec: type: description: Type represents the certificate type being extended, only ssh is supported at this time. - format: int32 - type: integer + x-kubernetes-int-or-string: true value: description: Value specifies the value to be used in the cert extension. @@ -1006,8 +1004,7 @@ spec: create_host_user_mode: description: CreateHostUserMode allows users to be automatically created on a host when not set to off - format: int32 - type: integer + x-kubernetes-int-or-string: true desktop_clipboard: description: DesktopClipboard indicates whether clipboard sharing is allowed between the user's workstation and the remote desktop. @@ -1118,8 +1115,7 @@ spec: require_session_mfa: description: RequireMFAType is the type of MFA requirement enforced for this user. - format: int32 - type: integer + x-kubernetes-int-or-string: true ssh_file_copy: description: SSHFileCopy indicates whether remote file operations via SCP or SFTP are allowed over an SSH session. It defaults @@ -2160,8 +2156,7 @@ spec: mode: description: Mode is the type of extension to be used -- currently critical-option is not supported - format: int32 - type: integer + x-kubernetes-int-or-string: true name: description: Name specifies the key to be used in the cert extension. @@ -2169,8 +2164,7 @@ spec: type: description: Type represents the certificate type being extended, only ssh is supported at this time. - format: int32 - type: integer + x-kubernetes-int-or-string: true value: description: Value specifies the value to be used in the cert extension. @@ -2203,8 +2197,7 @@ spec: create_host_user_mode: description: CreateHostUserMode allows users to be automatically created on a host when not set to off - format: int32 - type: integer + x-kubernetes-int-or-string: true desktop_clipboard: description: DesktopClipboard indicates whether clipboard sharing is allowed between the user's workstation and the remote desktop. @@ -2315,8 +2308,7 @@ spec: require_session_mfa: description: RequireMFAType is the type of MFA requirement enforced for this user. - format: int32 - type: integer + x-kubernetes-int-or-string: true ssh_file_copy: description: SSHFileCopy indicates whether remote file operations via SCP or SFTP are allowed over an SSH session. It defaults