customize entity descriptor for Google Workspace service provider

[flyinghermit]

In our effort to have pre-built catalog of SAML apps (service providers), we will have some preset configuration for each of the supported service providers. The first SAML app we are adding to catalog is Google Workspace. To have it configured, this PR:

• Adds a new package named samlserviceprovider under types. The package defines SAML service provider name constant for Google Workspace. This name will be used as a value for SAML service provider resource subkind. Subsequent name for other SAML apps, including their custom configuration values (if any) will go into this package.
• Updates SAML service provider entity descriptor generation flow to use subkind in order to customize entity descriptor values. In this PR, the only value that will be affected based on subkind is the entity descriptor NameID format.

part of https://github.com/gravitational/teleport.e/issues/2692

[espadolini]

Why use the subkind for this? The subkind should be used for things that need to be filtered or handled differently by subsystems that are not aware of the specifics of your type.

[flyinghermit]

Why use the subkind for this? The subkind should be used for things that need to be filtered or handled differently by subsystems that are not aware of the specifics of your type.

Isn't that what individual SAML service provider types are? They are supposed to be handled differently than the default saml_idp_service_provider kind.

[flyinghermit]

Why use the subkind for this? The subkind should be used for things that need to be filtered or handled differently by subsystems that are not aware of the specifics of your type.

In continuation to #37883 (comment)

The alternative here would be to have a constant value assigned to the SAMLIdPServiceProvider spec:

# using with subkind
kind: saml_idp_service_provider
sub-kind: google-workspace
metadata:
  name: appname
spec:
  entity_id: https://samlsp-host
  ...

# adding another knob in the SAMLIdPServiceProvider spec
kind: saml_idp_service_provider
metadata:
  name: appname
spec:
  type (or inernal_name?): google-workspace
  entity_id: https://samlsp-host

which I would want to avoid as we are exposing a knob that is used to handle the resource internally and has little to do with the SAMLIdPServiceProvider spec itself.

Additionally, we let users create multiple integration for a single service provider type, so cannot use the constant value in the metadata name as well, which has to be unique.

[espadolini]

The point of subkind (whatever little point it is, at least) is that it's a field that can be used at the watcher and cache layer for filtering; if that's not going to be a thing then we shouldn't be using the subkind for internal resource-specific flagging.

The alternative here would be to have a constant value assigned to the SAMLIdPServiceProvider spec which I would want to avoid as we are exposing a knob that is used to handle the resource internally and has little to do with the SAMLIdPServiceProvider spec itself.

The subkind is exposed right there in the resource exactly like an additional field in the spec would be, and current releases are not enforcing that the subkind is blank so any backwards compatibility concern on the additional field would apply in the exact same way on the subkind.

[flyinghermit]

The point of subkind (whatever little point it is, at least) is that it's a field that can be used at the watcher and cache layer for filtering; if that's not going to be a thing then we shouldn't be using the subkind for internal resource-specific flagging.

Gotcha. Though we have been using subkind for internal resource filtering. For example, PluginV1 which filters resources based on subkind access, mdm.

teleport/api/types/plugin.go

Lines 56 to 66 in 4e0968c

	type PluginSubkind string
	const (
	// PluginSubkindUnknown is returned when no plugin subkind matches.
	PluginSubkindUnknown PluginSubkind = ""
	// PluginSubkindMDM represents MDM plugins collectively
	PluginSubkindMDM = "mdm"
	// PluginSubkindAccess represents access request plugins collectively
	PluginSubkindAccess = "access"
	)

which is then passed to PluginV1 Spec https://github.com/gravitational/teleport.e/blob/master/lib/web/plugindescriptor.go#L123.

And also for the KindIntegration

teleport/api/types/integration.go

Lines 51 to 57 in dec9de9

	func NewIntegrationAWSOIDC(md Metadata, spec *AWSOIDCIntegrationSpecV1) (*IntegrationV1, error) {
	ig := &IntegrationV1{
	ResourceHeader: ResourceHeader{
	Metadata: md,
	Kind: KindIntegration,
	Version: V1,
	SubKind: IntegrationSubKindAWSOIDC,

[r0mant]

@flyinghermit @espadolini We need a way to differentiate between a generic SAML Service Provider and the one that represents GCP Web Console (and others in future, like Azure) because the behavior between those will be slightly different and they will show up separately in our Access Management UI. I don't want to introduce another resource for each specific SP, and we've used subkind in the past in similar cases (e.g. to differentiate between teleport vs openssh vs eic nodes), so I'm not sure why it would be a problem here? I personally think it's fine.

Another option is to have a "type: gcp" in the resource spec but that seems less "clean" than using a subkind as I think semantically it is exactly what it was introduced for. I wouldn't mind doing this either though if there's some caching problem this will cause we're not aware of.

[espadolini]

Both PluginV1 and IntegrationV1 rely on the subkind to decide how to JSONUnmarshal the spec; although that could've also been done with a dedicated top-level field in the spec, they truly are a container for different subtypes of resource. Cert authorities are all (mostly) the same internally, but each subkind (i.e. each CA type) is stored in a different key range, and we do filtering at the watcher level based on the type and name, which are available to the event watchers and fanouts because they're all part of the Resource interface (and as part of the backend key). ServerV2... shouldn't have been subkind either lol, it should've been "mode: agentless" or something like that in the spec.

Here we're not using the subkind to define a different format for the data or a different backend key range, and for all intents and purposes a "google-workspace" SAML IdP service provider is just a normal SAMP IdP, you could just create one pointing to something that's not google workspace just by setting all the correct fields, and the only difference (at least, right now) is at creation time; if all we're doing is setting some presets, why can't that be a "preset" field in the spec? Will there be further differences in how the type is defined as we add more presets?

if there's some caching problem this will cause

Likely not (I don't think people will ever be creating thousands of service providers), but let's put it this way: we only have one subkind field, and if we use it now it won't be available for storage/filtering purposes in the future, if the need arises.

[r0mant]

Sounds good, I don't have objections to having a preset: gcp field in the service provider resource spec either. cc @flyinghermit Let's do that.

[flyinghermit]

@espadolini @r0mant

My only reservation was that if this was idp kind, then preserving subkind for saml_idp and oidc_idp would exactly fit to custom filtering scenario as they would have different spec. But for saml_idp_service_provider which is already one level deep and there isn't going to be something like saml_idp_service_provider_v2 subkind. In that case, using subkind to define service provider preset doesn't seem like setting up a footgun here (unless it would really hurt our cache system)

But not a big deal, I am fine either way. Added new PR to have the preset field defined #37968 and tagged you both there.

Closing this PR and will follow up with another one to update entity descriptor configuration.