Skip to content

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

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.

Sign up for GitHub

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

Closed
justinas wants to merge 14 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8738331
Add Entra ID plugin boilerplate
Apr 12, 2024
2c3ce67
Add e imports for MS Graph SDK
Apr 12, 2024
84e1c4c
Add ability to sign Entra ID OIDC JWTs, rework KID handling
Apr 12, 2024
55ea750
Restore proto stuff lost in merges
Apr 15, 2024
8c96fcb
Add unit tests for Entra ID plugin validation
Apr 15, 2024
985bac0
Fix typo in assertion function name
Apr 15, 2024
a693e43
Update the OIDC JWKS test to expect the same key twice
Apr 15, 2024
aee903d
Add Entra ID plugin type constant
Apr 15, 2024
8670925
go mod tidy
Apr 15, 2024
ebb1288
Fix expected JWKS size in integration test
Apr 15, 2024
d6c698e
Merge branch 'master' into justinas/entra-id-boilerplate
justinas Apr 17, 2024
7f8551a
Add basic tests for KeyID
Apr 18, 2024
866a4e2
Move Azure auth settings from Plugin to Integration
Apr 24, 2024
1837b94
Merge remote-tracking branch 'origin/master' into justinas/entra-id-b…
Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion api/proto/teleport/legacy/types/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5781,10 +5781,12 @@ message PluginSpecV1 {
PluginServiceNowSettings serviceNow = 10;
// Settings for the Gitlab plugin.
PluginGitlabSettings gitlab = 12;
// Settings for the Entra ID plugin
PluginEntraIDSettings entra_id = 13;
}

// generation contains a unique ID that should:
// - Be created by the backed on plugin creation.
// - Be created by the backend on plugin creation.
// - Be updated by the backend if the plugin is updated in any way.
//
// For older plugins, it's possible for this to be empty.
Expand Down Expand Up @@ -5966,6 +5968,22 @@ message PluginDiscordSettings {
map<string, DiscordChannels> role_to_recipients = 1;
}

// PluginEntraIDSettings defines settings for the Entra ID sync plugin
message PluginEntraIDSettings {
option (gogoproto.equal) = true;
Copy link
Copy Markdown
Contributor

@tigrato tigrato Apr 17, 2024

Choose a reason for hiding this comment

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

Don't use gogo, use derive to generate the equal funcs

Copy link
Copy Markdown
Contributor

@tigrato tigrato Apr 18, 2024

Choose a reason for hiding this comment

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

Do we want to use it as a plugin or do we want to use it as an integration similarly to AWS?
I think the second is preferred as it's similar to aws

Copy link
Copy Markdown
Contributor Author

@justinas justinas Apr 18, 2024

Choose a reason for hiding this comment

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

Do we want to use it as a plugin or do we want to use it as an integration similarly to AWS?
I think the second is preferred as it's similar to aws

That's a good question. I looked into it, and I'm not fully sure what the role of an Integration is. Plugin came first, then Integration was created at some point. There is only a single Integration, that is AWS OIDC, but many Plugins. Integrations like Okta and Jamf run as either:

  • An independent service
  • A Plugin.

and I would argue they are closer to what we're implementing for Entra for now, as we're importing RBAC resources and not compute resources that the AWS integration mostly focuses on.

One benefit is that hosted plugins are now enabled for any enterprise deployment and the use of them does not require to spin up a discovery service instance.

Copy link
Copy Markdown
Contributor Author

@justinas justinas Apr 18, 2024

Choose a reason for hiding this comment

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

Don't use gogo, use derive to generate the equal funcs

I'm afraid all Plugin types are using gogoproto.equal currently, do we want to deviate from that at this point?

Copy link
Copy Markdown
Contributor

@jakule jakule Apr 18, 2024

Choose a reason for hiding this comment

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

I was thinking about the same, but then Okta is also defined as a plugin 🤷‍♂️

@r0mant ?

Copy link
Copy Markdown
Contributor Author

@justinas justinas Apr 18, 2024

Choose a reason for hiding this comment

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

I'm still failing to fully grasp the distinction, but I'll defer my judgement (let's see if Roman has any input).

Integration + Discovery Config workflow seems to have some rough UX edges, see https://github.com/gravitational/access-graph/issues/731 , though they are probably solvable.

Copy link
Copy Markdown
Contributor Author

@justinas justinas Apr 22, 2024

Choose a reason for hiding this comment

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

Had some time to think about this.

Entra being run as either a Plugin or a standalone host follows the established pattern for Jamf and Okta, as well as the upcoming GitLab TAG integration.

If we'd like to utilize the same "integration" credentials to discover Azure resources (which is out of scope for now), we only need Auth server to be able to utilize its information for signing the OIDC tokens - it should be able to do that whether the info is in an Integration or a Plugin.

IMO having this as a plugin is the most straightforward way. Let me know if I'm missing any hard blockers for this approach. @tigrato @jakule

Copy link
Copy Markdown
Contributor

@jakule jakule Apr 23, 2024

Choose a reason for hiding this comment

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

No preferences as long as it works really. @tigrato ?

Copy link
Copy Markdown
Contributor

@tigrato tigrato Apr 23, 2024

Choose a reason for hiding this comment

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

Can't we simply use an integration to provide access to Entra/Azure OIDC token - it's what an integration does as it doesn't run any code - while the sync code runs as a plugin. This would simplify future changes with supporting Azure discovery in the cloud as we don't need to maintain 2 pieces of code for the same thing.

My suggestion is:

  • add an integration: provides dynamic credentials signed by teleport to be used for azure/entra access
  • add a plugin: leverages the integration to generate the OIDC credentials it needs to access Entra and polls access lists from it and syncs to Teleport.

Copy link
Copy Markdown
Contributor Author

@justinas justinas Apr 24, 2024

Choose a reason for hiding this comment

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

@tigrato @jakule I've updated the PR by introducing an Azure OIDC integration type and moved the auth-related stuff there.

I did not expose a method for discovery to acquire OIDC tokens - will do that when we get there. For the plugin it is much easier to do that directly through Auth.


// SyncSettings controls the user and access list sync settings for EntraID.
PluginEntraIDSyncSettings sync_settings = 1;
}

// Defines settings for syncing users and access lists from Entra ID.
message PluginEntraIDSyncSettings {
option (gogoproto.equal) = true;

// DefaultOwners are the default owners for all imported access lists.
repeated string default_owners = 1;
}

message PluginBootstrapCredentialsV1 {
oneof credentials {
PluginOAuth2AuthorizationCodeCredentials oauth2_authorization_code = 1;
Expand Down Expand Up @@ -6422,6 +6440,8 @@ message IntegrationSpecV1 {
oneof SubKindSpec {
// AWSOIDC contains the specific fields to handle the AWS OIDC Integration subkind
AWSOIDCIntegrationSpecV1 AWSOIDC = 1 [(gogoproto.jsontag) = "aws_oidc,omitempty"];
// AzureOIDC contains the specific fields to handle the Azure OIDC Integration subkind
AzureOIDCIntegrationSpecV1 AzureOIDC = 2 [(gogoproto.jsontag) = "azure_oidc,omitempty"];
}
}

Expand All @@ -6440,6 +6460,17 @@ message AWSOIDCIntegrationSpecV1 {
string IssuerS3URI = 2 [(gogoproto.jsontag) = "issuer_s3_uri,omitempty"];
}

// AzureOIDCIntegrationSpecV1 contains the spec properties for the Azure OIDC SubKind Integration.
message AzureOIDCIntegrationSpecV1 {
// TenantID specifies the ID of Entra Tenant (Directory)
// that this plugin integrates with.
string TenantID = 1 [(gogoproto.jsontag) = "tenant_id,omitempty"];

// ClientID specifies the ID of Azure enterprise application (client)
// that corresponds to this plugin.
string ClientID = 2 [(gogoproto.jsontag) = "client_id,omitempty"];
}

// HeadlessAuthentication holds data for an ongoing headless authentication attempt.
message HeadlessAuthentication {
// Header is the resource header.
Expand Down
77 changes: 74 additions & 3 deletions api/types/integration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ import (
const (
// IntegrationSubKindAWSOIDC is an integration with AWS that uses OpenID Connect as an Identity Provider.
IntegrationSubKindAWSOIDC = "aws-oidc"

// IntegrationSubKindAzureOIDC is an integration with Azure that uses OpenID Connect as an Identity Provider.
IntegrationSubKindAzureOIDC = "azure-oidc"
)

// Integration specifies is a connection configuration between Teleport and a 3rd party system.
Expand All @@ -47,6 +50,9 @@ type Integration interface {
// SetAWSOIDCIssuerS3URI sets the IssuerS3URI of the AWS OIDC Spec.
// Eg, s3://my-bucket/my-prefix
SetAWSOIDCIssuerS3URI(string)

// GetAzureOIDCIntegrationSpec returns the `azure-oidc` spec fields.
GetAzureOIDCIntegrationSpec() *AzureOIDCIntegrationSpecV1
}

var _ ResourceWithLabels = (*IntegrationV1)(nil)
Expand All @@ -72,6 +78,27 @@ func NewIntegrationAWSOIDC(md Metadata, spec *AWSOIDCIntegrationSpecV1) (*Integr
return ig, nil
}

// NewIntegrationAzureOIDC returns a new `azure-oidc` subkind Integration
func NewIntegrationAzureOIDC(md Metadata, spec *AzureOIDCIntegrationSpecV1) (*IntegrationV1, error) {
ig := &IntegrationV1{
ResourceHeader: ResourceHeader{
Metadata: md,
Kind: KindIntegration,
Version: V1,
SubKind: IntegrationSubKindAzureOIDC,
},
Spec: IntegrationSpecV1{
SubKindSpec: &IntegrationSpecV1_AzureOIDC{
AzureOIDC: spec,
},
},
}
if err := ig.CheckAndSetDefaults(); err != nil {
return nil, trace.Wrap(err)
}
return ig, nil
}

// String returns the integration string representation.
func (ig *IntegrationV1) String() string {
return fmt.Sprintf("IntegrationV1(Name=%v, SubKind=%s, Labels=%v)",
Expand Down Expand Up @@ -128,14 +155,19 @@ func (s *IntegrationSpecV1) CheckAndSetDefaults() error {
if err != nil {
return trace.Wrap(err)
}
case *IntegrationSpecV1_AzureOIDC:
err := integrationSubKind.CheckAndSetDefaults()
if err != nil {
return trace.Wrap(err)
}
default:
return trace.BadParameter("unknown integration subkind: %T", integrationSubKind)
}

return nil
}

// CheckAndSetDefaults validates an agent mesh integration.
// CheckAndSetDefaults validates the configuration for AWS OIDC integration subkind.
func (s *IntegrationSpecV1_AWSOIDC) CheckAndSetDefaults() error {
if s == nil || s.AWSOIDC == nil {
return trace.BadParameter("aws_oidc is required for %q subkind", IntegrationSubKindAWSOIDC)
Expand All @@ -160,6 +192,21 @@ func (s *IntegrationSpecV1_AWSOIDC) CheckAndSetDefaults() error {
return nil
}

// CheckAndSetDefaults validates the configuration for Azure OIDC integration subkind.
func (s *IntegrationSpecV1_AzureOIDC) CheckAndSetDefaults() error {
if s == nil || s.AzureOIDC == nil {
return trace.BadParameter("azure_oidc is required for %q subkind", IntegrationSubKindAzureOIDC)
}
if s.AzureOIDC.TenantID == "" {
return trace.BadParameter("tenant_id must be set")
}
if s.AzureOIDC.ClientID == "" {
return trace.BadParameter("client_id must be set")
}

return nil
}

// GetAWSOIDCIntegrationSpec returns the specific spec fields for `aws-oidc` subkind integrations.
func (ig *IntegrationV1) GetAWSOIDCIntegrationSpec() *AWSOIDCIntegrationSpecV1 {
return ig.Spec.GetAWSOIDC()
Expand Down Expand Up @@ -198,6 +245,11 @@ func (ig *IntegrationV1) SetAWSOIDCIssuerS3URI(issuerS3URI string) {
}
}

// GetAzureOIDCIntegrationSpec returns the specific spec fields for `azure-oidc` subkind integrations.
func (ig *IntegrationV1) GetAzureOIDCIntegrationSpec() *AzureOIDCIntegrationSpecV1 {
return ig.Spec.GetAzureOIDC()
}

// Integrations is a list of Integration resources.
type Integrations []Integration

Expand Down Expand Up @@ -247,7 +299,8 @@ func (ig *IntegrationV1) UnmarshalJSON(data []byte) error {
d := struct {
ResourceHeader `json:""`
Spec struct {
AWSOIDC json.RawMessage `json:"aws_oidc"`
AWSOIDC json.RawMessage `json:"aws_oidc"`
AzureOIDC json.RawMessage `json:"azure_oidc"`
} `json:"spec"`
}{}

Expand All @@ -270,6 +323,17 @@ func (ig *IntegrationV1) UnmarshalJSON(data []byte) error {

integration.Spec.SubKindSpec = subkindSpec

case IntegrationSubKindAzureOIDC:
subkindSpec := &IntegrationSpecV1_AzureOIDC{
AzureOIDC: &AzureOIDCIntegrationSpecV1{},
}

if err := json.Unmarshal(d.Spec.AzureOIDC, subkindSpec.AzureOIDC); err != nil {
return trace.Wrap(err)
}

integration.Spec.SubKindSpec = subkindSpec

default:
return trace.BadParameter("invalid subkind %q", integration.ResourceHeader.SubKind)
}
Expand All @@ -290,7 +354,8 @@ func (ig *IntegrationV1) MarshalJSON() ([]byte, error) {
d := struct {
ResourceHeader `json:""`
Spec struct {
AWSOIDC AWSOIDCIntegrationSpecV1 `json:"aws_oidc"`
AWSOIDC AWSOIDCIntegrationSpecV1 `json:"aws_oidc"`
AzureOIDC AzureOIDCIntegrationSpecV1 `json:"azure_oidc"`
} `json:"spec"`
}{}

Expand All @@ -303,6 +368,12 @@ func (ig *IntegrationV1) MarshalJSON() ([]byte, error) {
}

d.Spec.AWSOIDC = *ig.GetAWSOIDCIntegrationSpec()
case IntegrationSubKindAzureOIDC:
if ig.GetAzureOIDCIntegrationSpec() == nil {
return nil, trace.BadParameter("missing subkind data for %q subkind", ig.SubKind)
}

d.Spec.AzureOIDC = *ig.GetAzureOIDCIntegrationSpec()
default:
return nil, trace.BadParameter("invalid subkind %q", ig.SubKind)
}
Expand Down
107 changes: 89 additions & 18 deletions api/types/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (
)

func TestIntegrationJSONMarshalCycle(t *testing.T) {
ig, err := NewIntegrationAWSOIDC(
aws, err := NewIntegrationAWSOIDC(
Metadata{Name: "some-integration"},
&AWSOIDCIntegrationSpecV1{
RoleARN: "arn:aws:iam::123456789012:role/DevTeams",
Expand All @@ -37,14 +37,29 @@ func TestIntegrationJSONMarshalCycle(t *testing.T) {
)
require.NoError(t, err)

bs, err := json.Marshal(ig)
azure, err := NewIntegrationAzureOIDC(
Metadata{Name: "some-integration"},
&AzureOIDCIntegrationSpecV1{
TenantID: "foo-bar",
ClientID: "baz-quux",
},
)
require.NoError(t, err)

var ig2 IntegrationV1
err = json.Unmarshal(bs, &ig2)
require.NoError(t, err)
allIntegrations := []*IntegrationV1{aws, azure}

require.Equal(t, &ig2, ig)
for _, ig := range allIntegrations {
t.Run(ig.SubKind, func(t *testing.T) {
bs, err := json.Marshal(ig)
require.NoError(t, err)

var ig2 IntegrationV1
err = json.Unmarshal(bs, &ig2)
require.NoError(t, err)

require.Equal(t, &ig2, ig)
})
}
}

func TestIntegrationCheckAndSetDefaults(t *testing.T) {
Expand All @@ -59,7 +74,7 @@ func TestIntegrationCheckAndSetDefaults(t *testing.T) {
expectedErrorIs func(error) bool
}{
{
name: "valid",
name: "aws-oidc: valid",
integration: func(name string) (*IntegrationV1, error) {
return NewIntegrationAWSOIDC(
Metadata{
Expand Down Expand Up @@ -104,9 +119,7 @@ func TestIntegrationCheckAndSetDefaults(t *testing.T) {
nil,
)
},
expectedErrorIs: func(err error) bool {
return trace.IsBadParameter(err)
},
expectedErrorIs: trace.IsBadParameter,
},
{
name: "aws-oidc: error when issuer is not a valid url",
Expand All @@ -121,9 +134,7 @@ func TestIntegrationCheckAndSetDefaults(t *testing.T) {
},
)
},
expectedErrorIs: func(err error) bool {
return trace.IsBadParameter(err)
},
expectedErrorIs: trace.IsBadParameter,
},
{
name: "aws-oidc: issuer is not an s3 url",
Expand All @@ -138,9 +149,7 @@ func TestIntegrationCheckAndSetDefaults(t *testing.T) {
},
)
},
expectedErrorIs: func(err error) bool {
return trace.IsBadParameter(err)
},
expectedErrorIs: trace.IsBadParameter,
},
{
name: "aws-oidc: error when no role is provided",
Expand All @@ -152,9 +161,71 @@ func TestIntegrationCheckAndSetDefaults(t *testing.T) {
&AWSOIDCIntegrationSpecV1{},
)
},
expectedErrorIs: func(err error) bool {
return trace.IsBadParameter(err)
expectedErrorIs: trace.IsBadParameter,
},
{
name: "azure-oidc: valid",
integration: func(name string) (*IntegrationV1, error) {
return NewIntegrationAzureOIDC(
Metadata{
Name: name,
},
&AzureOIDCIntegrationSpecV1{
ClientID: "baz-quux",
TenantID: "foo-bar",
},
)
},
expectedIntegration: func(name string) *IntegrationV1 {
return &IntegrationV1{
ResourceHeader: ResourceHeader{
Kind: KindIntegration,
SubKind: IntegrationSubKindAzureOIDC,
Version: V1,
Metadata: Metadata{
Name: name,
Namespace: defaults.Namespace,
},
},
Spec: IntegrationSpecV1{
SubKindSpec: &IntegrationSpecV1_AzureOIDC{
AzureOIDC: &AzureOIDCIntegrationSpecV1{
ClientID: "baz-quux",
TenantID: "foo-bar",
},
},
},
}
},
expectedErrorIs: noErrorFunc,
},
{
name: "azure-oidc: error when no tenant id is provided",
integration: func(name string) (*IntegrationV1, error) {
return NewIntegrationAzureOIDC(
Metadata{
Name: name,
},
&AzureOIDCIntegrationSpecV1{
ClientID: "baz-quux",
},
)
},
expectedErrorIs: trace.IsBadParameter,
},
{
name: "azure-oidc: error when no client id is provided",
integration: func(name string) (*IntegrationV1, error) {
return NewIntegrationAzureOIDC(
Metadata{
Name: name,
},
&AzureOIDCIntegrationSpecV1{
TenantID: "foo-bar",
},
)
},
expectedErrorIs: trace.IsBadParameter,
},
} {
t.Run(tt.name, func(t *testing.T) {
Expand Down
Loading