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

Added unique identifier to each request #1650

Merged
13 commits merged into from
Jul 27, 2016
Merged

Added unique identifier to each request #1650

13 commits merged into from
Jul 27, 2016

Conversation

ghost
Copy link

@ghost ghost commented Jul 25, 2016

Fixes #1617

@@ -4,14 +4,13 @@ import (
"encoding/json"
"fmt"
"regexp"
Copy link
Author

Choose a reason for hiding this comment

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

This file should not have been changed for this PR. Had an issue switching between branches.

@@ -1193,6 +1193,13 @@ func (ts *TokenStore) handleCreateCommon(
return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest
}

// Prevent internal policies from being assigned to any tokens
Copy link
Member

Choose a reason for hiding this comment

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

This should also be taken out.

@vishalnayak
Copy link
Member

Other then the comment of taking out the token_store changes from this PR, LGTM!

@@ -708,6 +708,12 @@ func (c *Core) sealInitCommon(req *logical.Request) (retErr error) {
return retErr
}

// Create an identifier for the request
var err error
req.ID, err = uuid.GenerateUUID()
Copy link
Member

Choose a reason for hiding this comment

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

I think we should do this as early as possible -- in http/logical.go in the buildLogicalRequest function. That way we can refer to it at any point in time, even if that's still in the http code. Right now we're only exposing it for auditing, but that could change in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, this comment should actually be for vault/request_handling.go -- when I was scrolling back up I didn't notice I was at the wrong place :-)

Copy link
Member

Choose a reason for hiding this comment

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

Although it applies here too. buildLogicalRequest is called in both handleSysStepDown and handleSysSeal, so by moving the UUID generation code there we don't need to duplicate it anywhere else.

@jefferai
Copy link
Member

There are a couple more things that need to be done here:

  1. In logical/sanitize.go function SanitizeResponse, the HTTPResponse struct needs to be updated to contain a RequestID (request_id).

  2. Once (1) is done, the field in the response over the wire has to actually be populated. The SanitizeResponse function only takes in a logical.Response. Probably the right thing to do would be to create a RequestID field in logical.Response and populate it. This will probably break nearly every single test though. It is probably sufficient to instead ensure that we populate the value after calling SanitizeResponse. This is in request_handling.go function wrapInCubbyhole where fortunately the logical.Request is available, so it's easy. The other spot is http/logical.go where we don't have logical.Request now, but we only ever call it from one location -- so we can simply pass in the request into respondLogical and use it to populate the HTTPResponse after the call.

  3. The current code won't actually write the ID into the audit logs. To do this, the structs in audit/format_json.go need to be updated, and then the code above that actually puts the values into the structs also has to be updated to copy the ID out of the request object and into the audit object.

@jefferai jefferai added this to the 0.6.1 milestone Jul 25, 2016
@@ -170,7 +170,9 @@ func respondLogical(w http.ResponseWriter, r *http.Request, path string, dataOnl
},
}
} else {
httpResp = logical.SanitizeResponse(resp)
sanitizedHttp := logical.SanitizeResponse(resp)
Copy link
Member

Choose a reason for hiding this comment

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

You can simply use httpResp here and then set the request ID that comes out, rather than creating a new variable and assigning afterwards.

Copy link
Author

Choose a reason for hiding this comment

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

I received an error in my attempt to set requestID directly because httpResp is an interface with no methods. If this isn't the way to handle it, I can change it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, cool -- you're right. I didn't look up enough in the function. No worries!

@jefferai
Copy link
Member

LGTM! 🚢

@ghost ghost merged commit ce6bc51 into master Jul 27, 2016
@ghost ghost deleted the request-uuid branch July 27, 2016 13:42
This pull request was closed.
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.

2 participants