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

Fix response wrapping from K/V version 2 #4511

Merged
merged 3 commits into from
May 10, 2018
Merged

Conversation

jefferai
Copy link
Member

@jefferai jefferai commented May 4, 2018

This takes place in two parts, since working on this exposed an issue
with response wrapping when there is a raw body set. The changes are (in
diff order):

  • A CurrentWrappingLookupFunc has been added to return the current
    value. This is necessary for the lookahead call since we don't want the
    lookahead call to be wrapped.

  • Support for unwrapping < 0.6.2 tokens via the API/CLI has been
    removed, because we now have backends returning 404s with data and can't
    rely on the 404 trick. These can still be read manually via
    cubbyhole/response.

  • KV preflight version request now ensures that its calls is not
    wrapped, and restores any given function after.

  • When responding with a raw body, instead of always base64-decoding a
    string value and erroring on failure, on failure we assume that it
    simply wasn't a base64-encoded value and use it as is.

  • A test that fails on master and works now that ensures that raw body
    responses that are wrapped and then unwrapped return the expected
    values.

  • A flag for response data that indicates to the wrapping handling that
    the data contained therein is already JSON decoded (more later).

  • RespondWithStatusCode now defaults to a string so that the value is
    HMAC'd during audit. The function always JSON encodes the body, so
    before now it was always returning []byte which would skip HMACing. We
    don't know what's in the data, so this is a "better safe than sorry"
    issue. If different behavior is needed, backends can always manually
    populate the data instead of relying on the helper function.

  • We now check unwrapped data after unwrapping to see if there were raw
    flags. If so, we try to detect whether the value can be unbase64'd. The
    reason is that if it can it was probably originally a []byte and
    shouldn't be audit HMAC'd; if not, it was probably originally a string
    and should be. In either case, we then set the value as the raw body and
    hit the flag indicating that it's already been JSON decoded so not to
    try again before auditing. Doing it this way ensures the right typing.

  • There is now a check to see if the data coming from unwrapping is
    already JSON decoded and if so the decoding is skipped before setting
    the audit response.

This takes place in two parts, since working on this exposed an issue
with response wrapping when there is a raw body set. The changes are (in
diff order):

* A CurrentWrappingLookupFunc has been added to return the current
value. This is necessary for the lookahead call since we don't want the
lookahead call to be wrapped.

* Support for unwrapping < 0.6.2 tokens via the API/CLI has been
removed, because we now have backends returning 404s with data and can't
rely on the 404 trick. These can still be read manually via
cubbyhole/response.

* KV preflight version request now ensures that its calls is not
wrapped, and restores any given function after.

* When responding with a raw body, instead of always base64-decoding a
string value and erroring on failure, on failure we assume that it
simply wasn't a base64-encoded value and use it as is.

* A test that fails on master and works now that ensures that raw body
responses that are wrapped and then unwrapped return the expected
values.

* A flag for response data that indicates to the wrapping handling that
the data contained therein is already JSON decoded (more later).

* RespondWithStatusCode now defaults to a string so that the value is
HMAC'd during audit. The function always JSON encodes the body, so
before now it was always returning []byte which would skip HMACing. We
don't know what's in the data, so this is a "better safe than sorry"
issue. If different behavior is needed, backends can always manually
populate the data instead of relying on the helper function.

* We now check unwrapped data after unwrapping to see if there were raw
flags. If so, we try to detect whether the value can be unbase64'd. The
reason is that if it can it was probably originally a []byte and
shouldn't be audit HMAC'd; if not, it was probably originally a string
and should be. In either case, we then set the value as the raw body and
hit the flag indicating that it's already been JSON decoded so not to
try again before auditing. Doing it this way ensures the right typing.

* There is now a check to see if the data coming from unwrapping is
already JSON decoded and if so the decoding is skipped before setting
the audit response.
@jefferai jefferai added this to the 0.10.2 milestone May 4, 2018
return nil, ErrInternalError
}
if resp.Data[logical.HTTPRawBodyAlreadyJSONDecoded] != nil {
delete(resp.Data, logical.HTTPRawBodyAlreadyJSONDecoded)
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, will the raw response be audited here? including the status code, and format?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and we will have attempted to restore []byte vs string.

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