Entra ID integration: add onboarding script by justinas · Pull Request #41811 · gravitational/teleport

justinas · 2024-05-21T13:09:29Z

Adds a new teleport integration configure subcommand to set up Azure OIDC integration. The command sets up:

An Entra ID enterprise application, both as an SSO connector to allow login with Entra to Teleport, and as a way for us to access Entra ID directory programatically (via OIDC auth).
Optionally, when access graph support is requested, queries information about existing enterprise applications from the "private" Azure API (see RFD for more info) and produces a JSON payload that the user is expected to upload when finishing the integration onboarding in the UI.

Also adds the web endpoint in the Proxy that generates the script wrapper for this.

Still missing:

Frontend to onboard Entra that utilizes this. Will come as a subsequent PR.

github-actions · 2024-05-21T16:58:25Z

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-05-23T14:14:15Z

+	// Get information about enterprise apps
+	appResp, err := graphClient.Applications().Get(ctx, nil)
+	if err != nil {
+		panic(err)


I don't think panicking here is a good idea.

jakule · 2024-05-23T14:16:06Z

+	_, err := io.Copy(writer, bytes.NewReader(src))
+	// We do not expect in-memory bytes I/O to fail.
+	if err != nil {
+		panic(err)


I'd still return the error here. The chances are close to 0, but we should not use panics in non critical code (unrecoverable failure that should stop the whole app).

jakule · 2024-05-23T14:17:35Z

+		return appID, tenantID, trace.Wrap(err)
+	}
+
+	displayName := "Teleport" + " " + strings.TrimPrefix(proxyPublicAddr, "https://")


nit: You can use url.Url to extract the hostname and make sure that the output is correct.

jakule · 2024-05-23T14:20:40Z

+	}
+
+	spList := spResponse.GetValue()
+	if len(spList) < 1 {


To make sure only one is returned?

Suggested change

if len(spList) < 1 {

if len(spList) != 1 {

jakule · 2024-05-23T14:23:36Z

+		slog.DebugContext(ctx, "trying token", "client_id", token.ClientID)
+		token, err := exchangeToken(ctx, tenantID, token)
+		if err != nil {
+			slog.ErrorContext(ctx, "error exchanging token", "err", err)


If incorrect token doesn't break the processing as another one can work this is not an error. I'd lower down the level to warning or even debug. If not token is found, an error is returned from this function anyway.

jakule · 2024-05-23T14:30:15Z

+	fmt.Println("Success! Use the following information to finish the integration onboarding in Teleport:")
+	fmt.Printf("Tenant ID: %s\nClient ID: %s\n", tenantID, appID)
+	if params.AccessGraph {
+		fmt.Println("Because Access Graph integration was enabled, we also created a file with the necessary data.")


I don't think that the fact that access graph is enabled is important. The important part is that something else needs to be done to finish the setup.

Suggested change

fmt.Println("Because Access Graph integration was enabled, we also created a file with the necessary data.")

fmt.Println("To finish the setup you will need `cache.json` file that we created for you.")

…nboarding-script

jakule

Approved with a few blocks to not block.

jakule · 2024-05-24T16:05:04Z

+	// AccessGraph is a flag indicating that access graph integration is requested.
+	// When this is true, the integration script will produce
+	// a cache file necessary for TAG synchronization.
+	AccessGraph bool


nit: I'd rename it to AccessGraphEnabled or something like that, but I also don't mind the current version.

jakule · 2024-05-24T16:06:13Z

+		}
+		sp, err := graphClient.ServicePrincipalsWithAppId(appID).Get(ctx, nil)
+		if err != nil {
+			slog.ErrorContext(ctx, "could not retrieve service principal", "app_id", *appID, "error", err)


This looks like a bug. If the error is not nil we should not try to call GetId() a few lines below.

jakule · 2024-05-24T16:07:19Z

+		spID := sp.GetId()
+		if spID == nil {
+			slog.WarnContext(ctx, "service principal ID is nil", "app_id", *appID)
+			continue


Instead of putting continue everywhere you can extract this logic to a separate function, return errors as expected and log them here in one place.

jakule · 2024-05-24T16:07:52Z

+
+		federatedSSOV2Compressed, err := gzipBytes(federatedSSOV2)
+		if err != nil {
+			slog.WarnContext(ctx, "can not compress the FederatedSsoV2 payload", "error", err)


The same as above. Is it fine to use federatedSSOV2Compressed if an error is returned?

jakule · 2024-05-24T16:12:40Z

+		return "", trace.Wrap(err)
+	}
+
+	resp, err := (&http.Client{}).Do(req)


Use defaults.HTTPClient() to use client with some "sane" defaults.

jakule · 2024-05-24T16:13:37Z

+	req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", accessToken))
+	req.Header.Add("x-ms-client-request-id", uuid.NewString())
+
+	resp, err := (&http.Client{}).Do(req)


Please use defaults.HTTPClient() to use client with custom config

jakule · 2024-05-24T16:14:28Z

+	if len(profile.Subscriptions) == 0 {
+		return "", trace.BadParameter("subscription not found")
+	}
+	return profile.Subscriptions[0].TenantID, nil


Please comment why other subscriptions are ignored.

jakule · 2024-05-24T16:17:03Z

@tigrato Can you please take a look?

…nboarding-script

* 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 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>

justinas force-pushed the justinas/entra-id-onboarding-script branch from c718ff9 to eb53161 Compare May 21, 2024 13:14

Add Entra ID integration onboarding script

d4fb0c1

justinas force-pushed the justinas/entra-id-onboarding-script branch from eb53161 to d4fb0c1 Compare May 21, 2024 13:20

Justinas Stankevicius added 4 commits May 21, 2024 16:55

Adapt after proto update

c19cf75

Validate names in azure script handler, add test

387d14b

Add license headers

6103cf9

Update Entra plugin test with SSO connector field

b87fc6f

justinas changed the title ~~Justinas/entra id onboarding script~~ Entra ID integration: add onboarding script May 21, 2024

Fix lint

4068049

justinas marked this pull request as ready for review May 21, 2024 16:57

justinas requested review from jakule and tigrato May 21, 2024 16:58

github-actions Bot added the size/lg label May 21, 2024

github-actions Bot requested review from GavinFrazar and ryanclark May 21, 2024 16:58

justinas added no-changelog Indicates that a PR does not require a changelog entry and removed size/lg labels May 21, 2024

GavinFrazar removed their request for review May 22, 2024 01:17

jakule reviewed May 23, 2024

View reviewed changes

Justinas Stankevicius added 6 commits May 24, 2024 18:46

Remove leftover panics

ee73034

Adjust success message

a3a2241

Downgrade log message level

65976a5

Expect exactly 1 SP for MS Graph, improve errors

abe0122

Properly extract hostname for enterprise app name

5baee10

Merge remote-tracking branch 'origin/master' into justinas/entra-id-o…

e766670

…nboarding-script

justinas requested a review from jakule May 24, 2024 15:58

jakule approved these changes May 24, 2024

View reviewed changes

tigrato approved these changes May 25, 2024

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from ryanclark May 25, 2024 12:58

Justinas Stankevicius added 7 commits May 27, 2024 12:34

Comment on assuming the first subscription

5a81455

Merge remote-tracking branch 'origin/master' into justinas/entra-id-o…

83f9e5e

…nboarding-script

Address review nits

c305d75

Factor out sso info fetch into a function

62dfd29

fixup refactor

40a72c4

Add retry logic to app role assignment

426531f

Make godoc conventional

1015c8b

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

Merged via the queue into master with commit ec8f212 May 27, 2024

justinas deleted the justinas/entra-id-onboarding-script branch May 27, 2024 19:29

	fmt.Println("Because Access Graph integration was enabled, we also created a file with the necessary data.")
	fmt.Println("To finish the setup you will need `cache.json` file that we created for you.")

Conversation

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

Uh oh!

Uh oh!

github-actions Bot commented May 21, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakule left a comment

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!

Uh oh!

jakule commented May 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

justinas commented May 21, 2024 •

edited

Loading