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

Add support to parameterize unauthenticated paths #12668

Merged
merged 25 commits into from Oct 13, 2021
Merged

Conversation

fairclothjm
Copy link
Contributor

@fairclothjm fairclothjm commented Sep 29, 2021

Description

This PR adds support to parameterize unauthenticated paths for a framework.Backend. This is needed to allow the following unauthenticated API paths for the upcoming OIDC Provider feature:

  • oidc/provider/+/.well-known/*
  • oidc/provider/+/token

Background

Today, unauthenticated paths get stored in the loginPaths in the router when the backend is mounted. The structure used is a radix tree which is ultimately stored in an atomic.Value.

The approach used in this PR is to store any unauthenticated paths containing wildcards in an array* of pre-split slices. All other unauthenticated paths (not containing wildcards) will continue to be stored in a radix tree. This PR introduces a new struct loginPathsEntry to hold both the array and radix tree.

*Note that this sits in the hot path of requests so we are micro-optimizing by storing pre-split slices of path segments.

Alternative approaches

Several alternative approaches were considered:

  • Store wildcard paths in the radix tree
    • the go-radix library does not handle wildcards in the middle of strings
    • an attempt was made to implement a custom Walk func for go-radix, however the lib doesn’t expose enough fields for this approach to be feasible
  • Use regexes for unauthenticated paths
    • this will break backwards compatibility since the glob * character that is currently used to indicate a prefix match has a different meaning as a regular expression

@vercel vercel bot temporarily deployed to Preview – vault September 29, 2021 19:30 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 29, 2021 19:30 Inactive
vault/router.go Outdated Show resolved Hide resolved
vault/router.go Outdated Show resolved Hide resolved
vault/router_test.go Show resolved Hide resolved
vault/router_test.go Outdated Show resolved Hide resolved
changelog/12668.txt Outdated Show resolved Hide resolved
@calvn
Copy link
Member

calvn commented Sep 30, 2021

Looks like CI caught a failure related to these changes.

@vercel vercel bot temporarily deployed to Preview – vault September 30, 2021 15:06 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 30, 2021 15:06 Inactive
If we ever encounter a mismatched segment, break and set a flag to
prevent false positives for prefix matches.

If it is a match we need to do a prefix check. But we should not return
unless HasPrefix also evaluates to true. Otherwise we should let the for
loop continue to check other possibilities and only return false once
all wildcard paths have been evaluated.
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 30, 2021 21:05 Inactive
@vercel vercel bot temporarily deployed to Preview – vault September 30, 2021 21:05 Inactive
@vercel vercel bot temporarily deployed to Preview – vault September 30, 2021 21:08 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 30, 2021 21:08 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 1, 2021 15:15 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 1, 2021 15:15 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 6, 2021 19:49 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 6, 2021 19:49 Inactive
sdk/logical/logical.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault October 6, 2021 19:55 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 6, 2021 19:55 Inactive
vault/router.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault October 6, 2021 21:27 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 6, 2021 21:27 Inactive
vault/router.go Outdated Show resolved Hide resolved
vault/router.go Outdated Show resolved Hide resolved
vault/router.go Outdated Show resolved Hide resolved
vault/router.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault October 7, 2021 17:13 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 7, 2021 17:13 Inactive
Copy link
Member

@calvn calvn left a comment

Choose a reason for hiding this comment

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

One last question, otherwise looks good!

vault/router.go Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 8, 2021 13:26 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 8, 2021 13:26 Inactive
Copy link
Member

@calvn calvn 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 to me, but it'd be good to get an additional approver on this one.

@calvn calvn added this to the 1.9 milestone Oct 8, 2021
Copy link
Member

@briankassouf briankassouf left a comment

Choose a reason for hiding this comment

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

Great work!

@fairclothjm fairclothjm merged commit 56c6f3c into main Oct 13, 2021
@fairclothjm fairclothjm deleted the unauth-paths-wildcard branch October 13, 2021 16:51
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.

None yet

5 participants