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.

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

Adds OIDC namespace_in_state option #140

Merged
merged 7 commits into from
Oct 29, 2020
Merged

Adds OIDC namespace_in_state option #140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3f86b61
Adds OIDC pass_namespace_in_state option
tvoran Oct 19, 2020
bbfa423
pass_namespace_in_state -> namespace_in_state
tvoran Oct 19, 2020
b021102
Only attempt to remove namespace if present
tvoran Oct 21, 2020
9166b30
Merge remote-tracking branch 'origin/master' into VAULT-570/pass-name…
tvoran Oct 22, 2020
fc3439a
default namespace_in_state to true
tvoran Oct 27, 2020
4ed5ad5
default to true only on new config write
tvoran Oct 28, 2020
58a9841
default to true in UI and extra comment
tvoran Oct 28, 2020
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
27 changes: 27 additions & 0 deletions path_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,14 @@ func pathConfig(b *jwtAuthBackend) *framework.Path {
Name: "Provider Config",
},
},
"namespace_in_state": {
Type: framework.TypeBool,
Description: "Pass namespace in the OIDC state parameter instead of as a separate query parameter. With this setting, the allowed redirect URL(s) in Vault and on the provider side should not contain a namespace query parameter. This means only one redirect URL entry needs to be maintained on the provider side for all vault namespaces that will be authenticating against it. Defaults to true for new configs.",
DisplayAttrs: &framework.DisplayAttributes{
Name: "Namespace in OIDC state",
Value: true,
},
},
},

Operations: map[logical.Operation]framework.OperationHandler{
Expand Down Expand Up @@ -166,6 +174,7 @@ func (b *jwtAuthBackend) pathConfigRead(ctx context.Context, req *logical.Reques
"jwks_ca_pem": config.JWKSCAPEM,
"bound_issuer": config.BoundIssuer,
"provider_config": config.ProviderConfig,
"namespace_in_state": config.NamespaceInState,
},
}

Expand All @@ -189,6 +198,23 @@ func (b *jwtAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Reque
ProviderConfig: d.Get("provider_config").(map[string]interface{}),
}

// Check if the config already exists, to determine if this is a create or
// an update, since req.Operation is always 'update' in this handler, and
// there's no existence check defined.
existingConfig, err := b.config(ctx, req.Storage)
if err != nil {
return nil, err
}
if nsInState, ok := d.GetOk("namespace_in_state"); ok {
config.NamespaceInState = nsInState.(bool)
} else if existingConfig == nil {
// new configs default to true
config.NamespaceInState = true
} else {
// maintain the existing value
config.NamespaceInState = existingConfig.NamespaceInState
}

// Run checks on values
methodCount := 0
if config.OIDCDiscoveryURL != "" {
Expand Down Expand Up @@ -349,6 +375,7 @@ type jwtConfig struct {
BoundIssuer string `json:"bound_issuer"`
DefaultRole string `json:"default_role"`
ProviderConfig map[string]interface{} `json:"provider_config"`
NamespaceInState bool `json:"namespace_in_state"`

ParsedJWTPubKeys []interface{} `json:"-"`
}
Expand Down
188 changes: 188 additions & 0 deletions path_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func TestConfig_JWT_Read(t *testing.T) {
"jwks_ca_pem": "",
"bound_issuer": "http://vault.example.com/",
"provider_config": map[string]interface{}{},
"namespace_in_state": false,
}

req := &logical.Request{
Expand Down Expand Up @@ -136,6 +137,7 @@ func TestConfig_JWT_Write(t *testing.T) {
OIDCResponseTypes: []string{},
BoundIssuer: "http://vault.example.com/",
ProviderConfig: map[string]interface{}{},
NamespaceInState: true,
}

conf, err := b.(*jwtAuthBackend).config(context.Background(), storage)
Expand Down Expand Up @@ -172,6 +174,7 @@ func TestConfig_JWKS_Update(t *testing.T) {
"jwt_supported_algs": []string{},
"bound_issuer": "",
"provider_config": map[string]interface{}{},
"namespace_in_state": false,
}

req := &logical.Request{
Expand Down Expand Up @@ -342,6 +345,7 @@ func TestConfig_OIDC_Write(t *testing.T) {
OIDCResponseTypes: []string{},
OIDCDiscoveryURL: "https://team-vault.auth0.com/",
ProviderConfig: map[string]interface{}{},
NamespaceInState: true,
}

conf, err := b.(*jwtAuthBackend).config(context.Background(), storage)
Expand Down Expand Up @@ -433,6 +437,7 @@ func TestConfig_OIDC_Write_ProviderConfig(t *testing.T) {
"provider": "azure",
"extraOptions": "abound",
},
NamespaceInState: true,
}

conf, err := b.(*jwtAuthBackend).config(context.Background(), storage)
Expand Down Expand Up @@ -489,6 +494,7 @@ func TestConfig_OIDC_Write_ProviderConfig(t *testing.T) {
OIDCResponseTypes: []string{},
OIDCDiscoveryURL: "https://team-vault.auth0.com/",
ProviderConfig: map[string]interface{}{},
NamespaceInState: true,
}

conf, err := b.(*jwtAuthBackend).config(context.Background(), storage)
Expand All @@ -502,6 +508,188 @@ func TestConfig_OIDC_Write_ProviderConfig(t *testing.T) {
})
}

func TestConfig_OIDC_Create_Namespace(t *testing.T) {
type testCase struct {
create map[string]interface{}
expected jwtConfig
}
tests := map[string]testCase{
"namespace_in_state not specified": {
create: map[string]interface{}{
"oidc_discovery_url": "https://team-vault.auth0.com/",
},
expected: jwtConfig{
OIDCDiscoveryURL: "https://team-vault.auth0.com/",
NamespaceInState: true,
OIDCResponseTypes: []string{},
JWTSupportedAlgs: []string{},
JWTValidationPubKeys: []string{},
ProviderConfig: map[string]interface{}{},
},
},
"namespace_in_state true": {
create: map[string]interface{}{
"oidc_discovery_url": "https://team-vault.auth0.com/",
"namespace_in_state": true,
},
expected: jwtConfig{
OIDCDiscoveryURL: "https://team-vault.auth0.com/",
NamespaceInState: true,
OIDCResponseTypes: []string{},
JWTSupportedAlgs: []string{},
JWTValidationPubKeys: []string{},
ProviderConfig: map[string]interface{}{},
},
},
"namespace_in_state false": {
create: map[string]interface{}{
"oidc_discovery_url": "https://team-vault.auth0.com/",
"namespace_in_state": false,
},
expected: jwtConfig{
OIDCDiscoveryURL: "https://team-vault.auth0.com/",
NamespaceInState: false,
OIDCResponseTypes: []string{},
JWTSupportedAlgs: []string{},
JWTValidationPubKeys: []string{},
ProviderConfig: map[string]interface{}{},
},
},
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
b, storage := getBackend(t)

req := &logical.Request{
Operation: logical.UpdateOperation,
Path: configPath,
Storage: storage,
Data: test.create,
}
resp, err := b.HandleRequest(context.Background(), req)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("err:%s resp:%#v\n", err, resp)
}

conf, err := b.(*jwtAuthBackend).config(context.Background(), storage)
assert.NoError(t, err)
assert.Equal(t, &test.expected, conf)
})
}

}

func TestConfig_OIDC_Update_Namespace(t *testing.T) {
type testCase struct {
existing map[string]interface{}
update map[string]interface{}
expected jwtConfig
}
tests := map[string]testCase{
"existing false, update to true": {
existing: map[string]interface{}{
"oidc_discovery_url": "https://team-vault.auth0.com/",
"namespace_in_state": false,
},
update: map[string]interface{}{
"oidc_discovery_url": "https://team-vault.auth0.com/",
"namespace_in_state": true,
},
expected: jwtConfig{
OIDCDiscoveryURL: "https://team-vault.auth0.com/",
NamespaceInState: true,
OIDCResponseTypes: []string{},
JWTSupportedAlgs: []string{},
JWTValidationPubKeys: []string{},
ProviderConfig: map[string]interface{}{},
},
},
"existing false, update something else": {
existing: map[string]interface{}{
"oidc_discovery_url": "https://team-vault.auth0.com/",
"namespace_in_state": false,
},
update: map[string]interface{}{
"oidc_discovery_url": "https://team-vault.auth0.com/",
"default_role": "ui",
},
expected: jwtConfig{
OIDCDiscoveryURL: "https://team-vault.auth0.com/",
NamespaceInState: false,
DefaultRole: "ui",
OIDCResponseTypes: []string{},
JWTSupportedAlgs: []string{},
JWTValidationPubKeys: []string{},
ProviderConfig: map[string]interface{}{},
},
},
"existing true, update to false": {
existing: map[string]interface{}{
"oidc_discovery_url": "https://team-vault.auth0.com/",
"namespace_in_state": true,
},
update: map[string]interface{}{
"oidc_discovery_url": "https://team-vault.auth0.com/",
"namespace_in_state": false,
},
expected: jwtConfig{
OIDCDiscoveryURL: "https://team-vault.auth0.com/",
NamespaceInState: false,
OIDCResponseTypes: []string{},
JWTSupportedAlgs: []string{},
JWTValidationPubKeys: []string{},
ProviderConfig: map[string]interface{}{},
},
},
"existing true, update something else": {
existing: map[string]interface{}{
"oidc_discovery_url": "https://team-vault.auth0.com/",
"namespace_in_state": true,
},
update: map[string]interface{}{
"oidc_discovery_url": "https://team-vault.auth0.com/",
"default_role": "ui",
},
expected: jwtConfig{
OIDCDiscoveryURL: "https://team-vault.auth0.com/",
NamespaceInState: true,
DefaultRole: "ui",
OIDCResponseTypes: []string{},
JWTSupportedAlgs: []string{},
JWTValidationPubKeys: []string{},
ProviderConfig: map[string]interface{}{},
},
},
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
b, storage := getBackend(t)

req := &logical.Request{
Operation: logical.UpdateOperation,
Path: configPath,
Storage: storage,
Data: test.existing,
}
resp, err := b.HandleRequest(context.Background(), req)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("err:%s resp:%#v\n", err, resp)
}

req.Data = test.update
resp, err = b.HandleRequest(context.Background(), req)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("err:%s resp:%#v\n", err, resp)
}

conf, err := b.(*jwtAuthBackend).config(context.Background(), storage)
assert.NoError(t, err)
assert.Equal(t, &test.expected, conf)
})
}

}

const (
testJWTPubKey = `-----BEGIN PUBLIC KEY-----
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEEVs/o5+uQbTjL3chynL4wXgUg2R9
Expand Down
22 changes: 22 additions & 0 deletions path_oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,24 @@ func (b *jwtAuthBackend) authURL(ctx context.Context, req *logical.Request, d *f
return logical.ErrorResponse("role %q could not be found", roleName), nil
}

// If namespace will be passed around in state, and it has been provided as
// a redirectURI query parameter, remove it from redirectURI, and append it
// to the state (later in this function)
namespace := ""
if config.NamespaceInState {
inputURI, err := url.Parse(redirectURI)
if err != nil {
return resp, nil
}
qParam := inputURI.Query()
namespace = qParam.Get("namespace")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we error here if it does contain a namespace (as opposed to ignoring/removing it)?

Copy link
Member Author

Choose a reason for hiding this comment

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

So in this block if namespace is a query parameter, it does need to be removed from the redirectURI, so it can be appended to state later on.

Though I did see that this was re-encoding the query parameters for redirectURI needlessly in the case where there are no namespaces to worry about. Updated in b021102.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for the explanation. Is the namespace always going to be passed as a query parameter regardless of NamespaceInState? How is the namespace determined if it's coming from a request header or request path?

Copy link
Member Author

Choose a reason for hiding this comment

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

So as things stand today with the redirect_uri, I think the namespace is always passed as a query parameter, since that's how the UI sends it to this auth_url endpoint. (The cli login for OIDC doesn't need to set a namespace parameter since it just opens a local listener.)

I did a little testing with the UI, and I don't see any headers coming through in req.Headers in the auth_url endpoint handler. I think it would be possible to infer the namespace from the mountpoint, or add another field to the auth_url API payload. Maybe that would be a cleaner way to specify the namespace? I'm not sure what else that would gain us, unless there's another UI-like system we're trying to integrate with.

if len(namespace) > 0 {
qParam.Del("namespace")
inputURI.RawQuery = qParam.Encode()
redirectURI = inputURI.String()
}
}

if !validRedirect(redirectURI, role.AllowedRedirectURIs) {
logger.Warn("unauthorized redirect_uri", "redirect_uri", redirectURI)
return resp, nil
Expand Down Expand Up @@ -408,6 +426,10 @@ func (b *jwtAuthBackend) authURL(ctx context.Context, req *logical.Request, d *f
logger.Warn("error generating OAuth state", "error", err)
return resp, nil
}
if config.NamespaceInState && len(namespace) > 0 {
// embed namespace in state in the auth_url
stateID = fmt.Sprintf("%s,ns=%s", stateID, namespace)
}

authCodeOpts := []oauth2.AuthCodeOption{
oidc.Nonce(nonce),
Expand Down
Loading