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

fix: use mount from path parameter given during authn #7

Merged
merged 4 commits into from
Jun 11, 2020
Merged

fix: use mount from path parameter given during authn #7

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab673e0
fix: use mount from path parameter given during authn
austingebauer May 27, 2020
8a7b224
fix: use regex for mount path segment matching
austingebauer May 29, 2020
4b0bd35
fix: check path segments using url path split
austingebauer Jun 1, 2020
b1222fd
fix: format of request-target header and validation
austingebauer Jun 11, 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
2 changes: 1 addition & 1 deletion backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ func TestBackEnd_ValidateInstancePrincipalLoginNonExistentRole(t *testing.T) {
func makeRequestAndValidateResponse(t *testing.T, cmdMap map[string]string, expectFailure bool, expectedTTL time.Duration, expectedPolicies []string) {

role := cmdMap["role"]
path := fmt.Sprintf(PathBaseFormat, role)
path := fmt.Sprintf(PathBaseFormat, "oci", role)
signingPath := PathVersionBase + path

backend, config, err := initTest(t)
Expand Down
7 changes: 6 additions & 1 deletion cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,18 @@ Configuration:
}

func (h *CLIHandler) Auth(c *api.Client, m map[string]string) (*api.Secret, error) {
mount, ok := m["mount"]
if !ok {
mount = "oci"
}

role, ok := m["role"]
if !ok {
return nil, fmt.Errorf("Enter the role")
}
role = strings.ToLower(role)

path := fmt.Sprintf(PathBaseFormat, role)
path := fmt.Sprintf(PathBaseFormat, mount, role)
signingPath := PathVersionBase + path

loginData, err := CreateLoginData(c.Address(), m, signingPath)
Expand Down
4 changes: 2 additions & 2 deletions path_login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
// These constants store the required http path & method information for validating the signed request
const (
PathVersionBase = "/v1"
PathBaseFormat = "/auth/oci/login/%s"
PathBaseFormat = "/auth/%s/login/%s"
PathLoginMethod = "get"
)

Expand Down Expand Up @@ -78,7 +78,7 @@ func (b *backend) pathLoginUpdate(ctx context.Context, req *logical.Request, dat
authenticateRequestHeaders := requestHeaders.(http.Header)

// Find the targetUrl and Method
finalLoginPath := PathVersionBase + fmt.Sprintf(PathBaseFormat, roleName)
finalLoginPath := PathVersionBase + fmt.Sprintf(PathBaseFormat, "oci", roleName)
Copy link
Contributor Author

@austingebauer austingebauer May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm looking for some feedback on this part of the code. The function requestTargetToMethodURL() is taking in the (request-target) header value along with an expected HTTP method and URL path.

The function verifies that:

  1. The (request-target) header exists
  2. The HTTP method in the header matches the expected HTTP method
  3. That URL path in the header matches the expected URL path

Is there any precedent for this type of verification? It looks like something using https://tools.ietf.org/html/draft-cavage-http-signatures-06#section-2.3 is being done.

Copy link
Contributor Author

@austingebauer austingebauer May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is problematic for this PR as we need to know the mount path segment in pathLoginUpdate() to construct finalLoginPath for the verification. Using "oci" as currently in this PR won't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vuhphamw Do you have any advice here?

Copy link

Choose a reason for hiding this comment

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

@austingebauer OCI REST API signature signing process requires the header includes (request-target) as described here https://docs.cloud.oracle.com/en-us/iaas/Content/API/Concepts/signingrequests.htm#five

In order to support mount path, we need to somehow pass the mount path to this function and validate the URL path bases on that, i.e. /auth/mounted-path/login/devRole. Is there a way to pass the mount path to this method via context or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @vuhphamw. Makes sense that the header needs to exist in order to comply with OCI API request signing. I don't think we can use a context here. The Vault CLI and other clients (e.g., curl) exercise this API over HTTP.

Is there any specific reason we're validating the value of the (request-target) from the POST body in pathLoginUpdate() instead of just letting OCI API verify the signature? It seems we're currently just checking the (request-target) value against some constants (path_login.go#L19) in the backend before sending them to the OCI API. Would checking that the (request-target) value in the POST body be non-empty or a regex suffice?

Copy link

Choose a reason for hiding this comment

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

@austingebauer Yes, I think regex is sufficient. Please ensure the target path match /auth/%s/login/%s. I believe it was just extra validation to ensure the header has right fields for signing process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vuhphamw I've updated this PR to use a regex to do the validation. Please have a look and let me know your thoughts.

I kept the actual login role in the regex. The only part of the URL path which has an open pattern is the mount path, which uses the ASCII character class [[:ascii:]]. I'm using that character class because of the limitation noted in the /sys/mounts documentation.

method, targetUrl, err := requestTargetToMethodURL(authenticateRequestHeaders[HdrRequestTarget], PathLoginMethod, finalLoginPath)
if err != nil {
return unauthorizedLogicalResponse(req, b.Logger(), err)
Expand Down