Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Azure OIDC accounts #2004

Merged
merged 10 commits into from
Nov 1, 2023
Merged

Azure OIDC accounts #2004

merged 10 commits into from
Nov 1, 2023

Conversation

benPearce1
Copy link
Contributor

@benPearce1 benPearce1 commented Sep 22, 2023

[sc-58116]

PR adds descriptions of main Azure OIDC functionality and refactors the supplied scripts to pull the common bits (create service principal) and specific bits (create password for service principal, create federated creds for service principal)
Adds overview of configuration for OIDC
Adds brief overview of signing keys

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #58116: Docs.

@benPearce1 benPearce1 changed the title initial unfinished work Azure OIDC accounts Nov 1, 2023
@benPearce1 benPearce1 marked this pull request as ready for review November 1, 2023 02:07
<details data-group="infrastructure-accounts-azure">
<summary>Az PowerShell</summary>

```powershell
# This script will create a new service principal for you to use in Octopus Deploy using the Az PowerShell modules. This will work with both PowerShell and PowerShell Core.
# this script will create a new Azure AD App Registration

$AzureTenantId = "2a681dca-3230-4e01-abcb-b1fd225c0982" # Replace with your Tenant Id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Is there a reason this has the tenant specified? This is a pre-existing part of the docs so I'm happy for it to be left as is, the inconsistency is just a bit odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the Azure Active Directory tenant , not an Octopus one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️

Copy link
Contributor

@IsaacCalligeros95 IsaacCalligeros95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of questions/suggestions

  • Should we mention the Octopus.OpenIdConnect.Jwt variable that gets set and used for token exchange? This could be useful for customers looking to handle their own token exchanges in scripts.

  • Can we mention supported tool versions somewhere? In particular for terraform the azurerm terraform provider we require a minimum version of 3.22. Azure cli added support for MSAL in 2.30 which is required for the az login ... --federated-token command

@@ -22,9 +22,13 @@ These must be exposed with anonymous access on HTTPS. Without this, the OpenID C

The hostname of the URL that these two endpoints are available on must either be configured under **Configuration->Nodes->Server Uri** or set as the first ListenPrefix in the server configuration.

## Authenticating using OpenID Connect with third party services and tools

If you have a third-party service or tool that supports OpenID Connect, you can add any OIDC account variable into your projects variable set and use the `Octopus.OpenIDConnect.Jwt` variable to get access to the request token that can be used for authenticating.
Copy link
Contributor

@IsaacCalligeros95 IsaacCalligeros95 Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I was mistaken before, when using the variables it should be <AccountName>.OpenIDConnect.Jwt

Copy link
Contributor

@IsaacCalligeros95 IsaacCalligeros95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's just the variable name to update when using project variables, other than that LGTM

Copy link
Contributor

@hnrkndrssn hnrkndrssn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit but looks good otherwise.

src/pages/docs/deployments/azure/ase/index.md Outdated Show resolved Hide resolved
Co-authored-by: Henrik Andersson <henrik.andersson@octopus.com>
@benPearce1 benPearce1 merged commit 991615d into main Nov 1, 2023
4 checks passed
@benPearce1 benPearce1 deleted the bp/oidc-client branch November 1, 2023 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants