Entra ID integration boilerplate by justinas · Pull Request #40998 · gravitational/teleport

justinas · 2024-04-29T13:27:18Z

Split off from #40556 . Depends on #40997

Introduces validation for types introduced in #40997 , as well as modifications to OIDC/JWT logic required to sign tokens for Entra APIs. See #40556 for more details.

- Synthesize Key IDs for our JWT keys. For backwards compatibility, also include the same keys with an empty `kid` in JWKS. - Sign AWS OIDC tokens with a `kid=""` header claim, rather than omitting the `kid` claim altogether. See comment for details.

github-actions · 2024-04-29T13:53:31Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

jakule · 2024-04-29T16:28:32Z

 		ResourceHeader `json:""`
 		Spec           struct {
-			AWSOIDC AWSOIDCIntegrationSpecV1 `json:"aws_oidc"`
+			AWSOIDC   AWSOIDCIntegrationSpecV1   `json:"aws_oidc"`


You should probably add omitempty since now only one will be set

jakule · 2024-04-29T16:29:17Z

-			expectedErrorIs: func(err error) bool {
-				return trace.IsBadParameter(err)
-			},
+			expectedErrorIs: trace.IsBadParameter,


jakule · 2024-04-29T16:31:19Z

+
+	token, err := privateKey.SignEntraOIDC(jwt.SignParams{
+		Audience: azureDefaultJWTAudience,
+		Subject:  "teleport-azure", // TODO(justinas): consider moving this to a constant or a field in the integration settings


Should this be addressed before merging?

Shouldn't be a problem later: as long as we have no user-facing way to onboard this, there's nothing to break 🙂 Left it TODO as I'm not sure how to handle this, but considering 1 ID provider = 1 subject for purposes of Entra app auth, constant should be okay I guess.

jakule · 2024-04-29T16:31:55Z

 }
+
+func TestKeyID(t *testing.T) {
+	t.Run("deterministic", func(t *testing.T) {


Those can be two separate tests

jakule · 2024-04-29T16:32:09Z

 	require.Equal(t, "sig", jwk.Use)
 }
+
+func TestKeyID(t *testing.T) {


Suggested change

func TestKeyID(t *testing.T) {

func TestKeyID(t *testing.T) {

t.Parallel()

strideynet · 2024-04-30T09:26:21Z

 	require.Equal(t, "sig", jwk.Use)
 }
+
+func TestKeyIDHasConsistentOutputForAnInput(t *testing.T) {


It'd be nice to see a test which takes a known public key and asserts that it produces a fixed KeyID - that'd let us ensure that we don't accidentally introduce a regression that causes the key ID in an already issued JWT to mismatch the key ID in the jwks.

Good idea, added in c44c7c2.

tigrato · 2024-05-02T10:42:54Z

 			return trace.Wrap(err)
 		}
+	case *IntegrationSpecV1_AzureOIDC:
+		err := integrationSubKind.CheckAndSetDefaults()


Suggested change

err := integrationSubKind.CheckAndSetDefaults()

err := integrationSubKind.Validate()

tigrato · 2024-05-02T10:43:13Z

+// CheckAndSetDefaults validates the configuration for Azure OIDC integration subkind.
+func (s *IntegrationSpecV1_AzureOIDC) CheckAndSetDefaults() error {


Suggested change

// CheckAndSetDefaults validates the configuration for Azure OIDC integration subkind.

func (s *IntegrationSpecV1_AzureOIDC) CheckAndSetDefaults() error {

// Validate validates the configuration for Azure OIDC integration subkind.

func (s *IntegrationSpecV1_AzureOIDC) Validate() error {

tigrato · 2024-05-02T10:43:59Z

 	return nil
 }

+func (c *PluginEntraIDSettings) CheckAndSetDefaults() error {


Suggested change

func (c *PluginEntraIDSettings) CheckAndSetDefaults() error {

func (c *PluginEntraIDSettings) Validate() error {

* Add e imports for MS Graph SDK * Add ability to sign Entra ID OIDC JWTs, rework KID handling - Synthesize Key IDs for our JWT keys. For backwards compatibility, also include the same keys with an empty `kid` in JWKS. - Sign AWS OIDC tokens with a `kid=""` header claim, rather than omitting the `kid` claim altogether. See comment for details. * Add validation for Entra ID plugin * Fix typo in assertion function name * Update the OIDC JWKS test to expect the same key twice * Add Entra ID plugin type constant * go mod tidy * Fix expected JWKS size in integration test * Add basic tests for KeyID * Move Azure auth settings from Plugin to Integration * Address review comments * Add a unit test to ensure KeyID compatibility * Add license header to token_generator.go * Rename validation function per new conventions

* Entra ID reconciler: directory reconciler prerequisites (#40778) * Add Entra ID resource origin * Ignore ID and Revision from `header` in cmp * Add e_imports for MS Graph SDK * Entra ID integration: add proto definitions (#40997) * Entra ID integration boilerplate (#40998) * Add e imports for MS Graph SDK * Add ability to sign Entra ID OIDC JWTs, rework KID handling - Synthesize Key IDs for our JWT keys. For backwards compatibility, also include the same keys with an empty `kid` in JWKS. - Sign AWS OIDC tokens with a `kid=""` header claim, rather than omitting the `kid` claim altogether. See comment for details. * Add validation for Entra ID plugin * Fix typo in assertion function name * Update the OIDC JWKS test to expect the same key twice * Add Entra ID plugin type constant * go mod tidy * Fix expected JWKS size in integration test * Add basic tests for KeyID * Move Azure auth settings from Plugin to Integration * Address review comments * Add a unit test to ensure KeyID compatibility * Add license header to token_generator.go * Rename validation function per new conventions * Access Graph: sync AWS identity providers (#41368) * Add AWSSAMLProviderV1 to access graph proto * Access Graph: sync AWS SAML Providers * Parse SAML entity descriptor before sending to TAG * Add protos for AWS OIDC providers * Fetch AWS OIDC providers * Fetch signing certificates for AWS SAML providers * Deflake identity provider fetch test The concrete implementation of IAM mock uses a map, resulting in non-deterministic iteration order. Sort the results before comparing to alleviate. * Update lib/srv/discovery/fetchers/aws-sync/iam_test.go Co-authored-by: Jakub Nyckowski <jakub.nyckowski@goteleport.com> --------- Co-authored-by: Jakub Nyckowski <jakub.nyckowski@goteleport.com> * Access Graph: Entra ID application sync prerequisites (#41650) * Add access graph settings to Entra ID plugin * Move Entra ID labels to OSS * Add Entra resources and RPC to Access Graph proto * Add azure-oidc integration to web. Current code assumes that Integration is always either AwsOidc, or an external audit storage integration * Change app sso cache to a repeated field * Entra ID integration: add onboarding script (#41811) * Add Entra ID integration onboarding script * Adapt after proto update * Validate names in azure script handler, add test * Add license headers * Update Entra plugin test with SSO connector field * Fix lint * Remove leftover panics * Adjust success message * Downgrade log message level * Expect exactly 1 SP for MS Graph, improve errors * Properly extract hostname for enterprise app name * Comment on assuming the first subscription * Address review nits * Factor out sso info fetch into a function * fixup refactor * Add retry logic to app role assignment * Make godoc conventional * Entra ID integration: integration script updates and web onboarding prerequisites (#42172) * Remove integration name validation from web script Not used by the script. It is validated by the "plugins/validate" endpoint. * Add required frontend constants for Entra ID * Support Azure/Entra integrations in the list * Add IsPolicyEnabled to web config * Allow custom URL for ButtonLockedFeature * Add CTA_ENTRA_ID event type * Expose TAGInfoCache for use in e * Add LackingIgs option * Add Entra ID icon * Add Entra ID plugin to storybook * Bump e for dev build * Return underlying error in getPrivateAPIToken * Find default Azure subscription instead of the first one * Require user to re-login when provisioning Azure OIDC * Update prehog protos with Entra ID values From https://github.com/gravitational/cloud/pull/9111 * Suppress verbose warnings / information from az * Add an additional message after successful auth Lets user know that `az login` has completed and `teleport` is continuing its work. * Move EntraId constant to the bottom * Revert unintended changes to usageevents CTA is 1-to-1 with prehog, but IntegrationEnrollKind is not. * Remove integrationName validation asserts from test This parameter is no longer accepted by the endpoint * Revert "Bump e for dev build" This reverts commit fc747a0. * `go mod tidy` secondary modules --------- Co-authored-by: Jakub Nyckowski <jakub.nyckowski@goteleport.com>

Some JWT libraries panic with multiple keys present in the JWKS. A second JWKS key entry was added in #40998 Fixes #44245

* Set the key id in JWT Some JWT libraries panic with multiple keys present in the JWKS. A second JWKS key entry was added in #40998 Fixes #44245 * Update lib/jwt/jwt.go Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> * fix compile error --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

* Set the key id in JWT Some JWT libraries panic with multiple keys present in the JWKS. A second JWKS key entry was added in #40998 Fixes #44245 * Update lib/jwt/jwt.go * fix compile error --------- Co-authored-by: Jeff Anderson <jeff@goteleport.com> Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

Justinas Stankevicius added 10 commits April 29, 2024 16:27

Add e imports for MS Graph SDK

9937b31

Add validation for Entra ID plugin

98d17e7

Fix typo in assertion function name

a11c750

Update the OIDC JWKS test to expect the same key twice

9de2740

Add Entra ID plugin type constant

e3b8534

go mod tidy

dae7c20

Fix expected JWKS size in integration test

527df59

Add basic tests for KeyID

d80c1b5

Move Azure auth settings from Plugin to Integration

8e8232a

justinas force-pushed the justinas/entra-id-boilerplate2 branch from fec6c58 to 8e8232a Compare April 29, 2024 13:27

justinas changed the title ~~Justinas/entra id boilerplate2~~ Justinas: Entra ID integration boilerplate Apr 29, 2024

justinas marked this pull request as ready for review April 29, 2024 13:53

justinas requested a review from jakule April 29, 2024 13:53

github-actions Bot requested review from rudream and strideynet April 29, 2024 13:53

justinas requested review from tigrato and removed request for rudream and strideynet April 29, 2024 13:53

github-actions Bot added the size/md label Apr 29, 2024

justinas added the no-changelog Indicates that a PR does not require a changelog entry label Apr 29, 2024

justinas mentioned this pull request Apr 29, 2024

Entra ID integration: add plugin boilerplate and OIDC adjustments #40556

Closed

justinas changed the title ~~Justinas: Entra ID integration boilerplate~~ Entra ID integration boilerplate Apr 29, 2024

jakule approved these changes Apr 29, 2024

View reviewed changes

Address review comments

91000ea

strideynet approved these changes Apr 30, 2024

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from tigrato April 30, 2024 09:28

Justinas Stankevicius and others added 2 commits April 30, 2024 15:43

Add a unit test to ensure KeyID compatibility

c44c7c2

Merge branch 'master' into justinas/entra-id-boilerplate2

8f7a15f

Add license header to token_generator.go

d7935ee

tigrato approved these changes May 2, 2024

View reviewed changes

Rename validation function per new conventions

9b953cf

justinas added this pull request to the merge queue May 2, 2024

Merged via the queue into master with commit 68886bb May 2, 2024

justinas deleted the justinas/entra-id-boilerplate2 branch May 2, 2024 14:03

justinas mentioned this pull request May 8, 2024

Fix integration filtering in AwsAccount selector #41329

Merged

programmerq added a commit that referenced this pull request Aug 9, 2024

Set the key id in JWT

ea73afd

Some JWT libraries panic with multiple keys present in the JWKS. A second JWKS key entry was added in #40998 Fixes #44245

programmerq mentioned this pull request Aug 9, 2024

Set the key id in JWT #45350

Merged

programmerq added a commit that referenced this pull request Aug 9, 2024

Set the key id in JWT

7e404c9

Some JWT libraries panic with multiple keys present in the JWKS. A second JWKS key entry was added in #40998 Fixes #44245

programmerq added a commit that referenced this pull request Aug 12, 2024

Set the key id in JWT

d931c71

Some JWT libraries panic with multiple keys present in the JWKS. A second JWKS key entry was added in #40998 Fixes #44245

programmerq added a commit that referenced this pull request Aug 19, 2024

Set the key id in JWT

c76c2fa

Some JWT libraries panic with multiple keys present in the JWKS. A second JWKS key entry was added in #40998 Fixes #44245

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entra ID integration boilerplate#40998

Entra ID integration boilerplate#40998
justinas merged 15 commits intomasterfrom
justinas/entra-id-boilerplate2

justinas commented Apr 29, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 29, 2024

Uh oh!

jakule Apr 29, 2024

Uh oh!

jakule Apr 29, 2024

Uh oh!

jakule Apr 29, 2024

Uh oh!

justinas Apr 29, 2024

Uh oh!

jakule Apr 29, 2024

Uh oh!

jakule Apr 29, 2024

Uh oh!

strideynet Apr 30, 2024

Uh oh!

justinas Apr 30, 2024

Uh oh!

tigrato May 2, 2024

Uh oh!

tigrato May 2, 2024

Uh oh!

tigrato May 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	func TestKeyID(t *testing.T) {
	func TestKeyID(t *testing.T) {
	t.Parallel()

	err := integrationSubKind.CheckAndSetDefaults()
	err := integrationSubKind.Validate()

		// CheckAndSetDefaults validates the configuration for Azure OIDC integration subkind.
		func (s *IntegrationSpecV1_AzureOIDC) CheckAndSetDefaults() error {

	func (c *PluginEntraIDSettings) CheckAndSetDefaults() error {
	func (c *PluginEntraIDSettings) Validate() error {

Conversation

justinas commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justinas commented Apr 29, 2024 •

edited

Loading