-
Notifications
You must be signed in to change notification settings - Fork 123
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
Ofira jwt OIDC header1 #2637
Ofira jwt OIDC header1 #2637
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oburstein-hub thanks, please take a look at my comments.
Can you please elaborate more precisely in the description of the PR about the change? For example, the fallback mechanism between the body and header and the fact that we look for the Authorization: Bearer {token} structure specifically.
Also, it's missing the Jira issue it fixes and you also didn't update the checkboxes. Please fill those as well.
Lastly, please don't forget to organize the commits once all the other changes are in and it's ready
app/domain/authentication/authn_oidc/update_input_with_username_from_id_token.rb
Outdated
Show resolved
Hide resolved
@@ -12,7 +12,7 @@ module AuthnOidc | |||
) do | |||
extend(Forwardable) | |||
def_delegators(:@authenticator_input, :service_id, :authenticator_name, | |||
:account, :username, :webservice, :credentials, :client_ip, | |||
:account, :username, :webservice, :credentials, :client_ip, :request, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the request
and credentials
overlapping? The credentials
value is supposed to be taken from the body of the request, but then we send the entire request as another parameter, which contains the body as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This request parameter is generic (common) to all types of authenticators. If I will change it - I will damage all other authenticators. So - I think we should not touch it. WDYT ? @rafis3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oburstein-hub, avoiding a cascading change feels like the right tradeoff. Mind adding a comment as to why we're adding the request
object (so future engineers understand the decision)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comment
@@ -42,23 +45,35 @@ def request_body(request) | |||
end | |||
|
|||
let(:authenticate_id_token_request) do | |||
mock_authenticate_oidc_request(request_body_data: "id_token={\"id_token_username_field\": \"alice\"}") | |||
mock_authenticate_oidc_request(request_body_data: "id_token={\"id_token_username_field\": \"alice\"}", request_headers: {"CONTENT_TYPE" => "text/plain"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the test need this new content type? Doesn't that mean the API is broken from how it used to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
end | ||
|
||
let(:authenticate_id_token_request_id_token_in_header_field) do | ||
mock_authenticate_oidc_request(request_body_data: "id_token=", request_headers: {"HTTP_AUTHORIZATION" => "Bearer {\"id_token_username_field\":\"alice\"}"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are missing tests where the id_token
key is not present in the body at all, that the body is totally empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the Bearer
value come through as a Base64 encoded JWT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw the comment below. Maybe one of these is Base64 encoded to ensure the typical flow is tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The token is base64 encoded, we implemented same test as the id_token
present in the body, will create integration test for Conjur cloud UI in different story
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add at least a unit test here as well. I'm not comfortable adding functionality without tests to back it up. The Core team will be on the line for supporting this work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added integration test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
end | ||
|
||
let(:authenticate_id_token_request_invalid_id_token_in_header_field) do | ||
mock_authenticate_oidc_request(request_body_data: "id_token=", request_headers: {"HTTP_AUTHORIZATION" => "{\"id_token_username_field\":\"alice\"}"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I see that the given token is expected to be in cleartext and not in base64. Will that work for our new flow we need to support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - they both work - I have checked
@@ -247,6 +262,108 @@ def request_body(request) | |||
expect { subject }.to raise_error(::Errors::Authentication::RequestBody::MissingRequestParam) | |||
end | |||
end | |||
|
|||
context "with valid id token in header" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test also puts the token in the body. The name above implies that it will check the token from the header but that won't be the case. Am I right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will check token from header. it is missing in bode in this case
end | ||
end | ||
|
||
context "with valid id token in header" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This context has the exact name as above. Can you update it to describe its true purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted it
end | ||
end | ||
|
||
context "with invalid id token in header" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also in body, not just in header. Can you please update the description to be more accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
end | ||
end | ||
|
||
context "with empty id token in header" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, can you please update this description also with the state of the body of this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
username: nil, | ||
credentials: request_body(authenticate_id_token_request_missing_id_token_field), | ||
client_ip: '127.0.0.1', | ||
request: authenticate_id_token_request_empty_id_token_in_header_field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a test that checks that the Authorization header exists, with a value of Bearer but nothing after it?
Another test that I would add, is that there is an Authorization header, and the first word is not Bearer, while the second word is a valid token (should fail)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ae214b7
to
be2951f
Compare
) | ||
|
||
::Authentication::AuthnOidc::UpdateInputWithUsernameFromIdToken.new( | ||
verify_and_decode_token: mocked_decode_and_verify_id_token, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mocked_decode_and_verify_id_token
mock accepts and parses JSON: https://github.com/cyberark/conjur/blob/master/spec/app/domain/authentication/authn-oidc/update_input_with_username_from_id_token_spec.rb#L18.
It looks like this will fail if we pass Base64 encoded JSON. When the bearer token is passed in the body, is it passed as JSON?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we implemented same test as the id_token
in the body.
if decoded_credentials.fetch(id_token_field_name, "") == "" | ||
raise Errors::Authentication::RequestBody::MissingRequestParam, id_token_field_name | ||
def authorization_header_token(headers) | ||
@id_token = if headers&.key?("HTTP_AUTHORIZATION") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @
denotes an instance variable, which means it's available outside the method. Unless we need @id_token
outside this method, please use the local variable id_token
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
def decoded_credentials | ||
@decoded_credentials ||= Hash[URI.decode_www_form(credentials)] | ||
id_token_field_name = "id_token" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to this change, this method was a memoization method (the instance variable @decoded_credentials
stored the value during the life of the class). If you'd like to maintain the memoization, this method can be rewritten like:
def decoded_credentials
@decoded_credentials ||= begin
credential_token = Hash[URI.decode_www_form(credentials)]
if credential_token.fetch('id_token', "").empty?
Hash[URI.decode_www_form("id_token=#{authorization_header_token(request.headers)}")]
else
credential_token
end
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
be2951f
to
68cf01a
Compare
## [1.18.3] - 2022-09-05 | ||
|
||
### Added | ||
- Adds support for authorization token in header in OIDC authenticator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lists should be surrounded by blank lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aloncarmel111 , most of this looks good, thanks! I had one suggestion for trying to make the logic a little easier to read/follower. Let me know what you think!
app/domain/authentication/authn_oidc/update_input_with_username_from_id_token.rb
Show resolved
Hide resolved
f2906cd
to
b2d52f6
Compare
Thanks @aloncarmel111 , the only blocker I have left is to squash the PR review commits into the one It looks like there are still unresolved discussions around base64 encoding the header in the tests with Jason and Rafi. So you can wait to do that squash until that is resolved, and then ping me for approval. Thanks! |
end | ||
end | ||
|
||
def token_from_header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complex method Authentication::AuthnOidc#token_from_header (21.3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the move to break up these methods to better cover edge cases. I'd suggest passing the header in as an argument:
def token_from_header | |
def token_from_header(header) |
Passing the header into the method will greatly simplify writing tests (which it looks like need to be written).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest passing the header in as an argument:
These are internal private methods, so we wouldn't expect them to have unit tests for themselves. Rather, request
is the input that would need to be manipulated for the public interface of UpdateInputWithUsernameFromIdToken
. Adding parameters to these methods means we're passing internal class state between internal class methods. No?
If we want to unit tests these separately, that should be factored out and injected as dependencies here instead. I don't know that that's warranted in this case, but I don't think we should add parameters to them as class methods now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @micahlee. I'm ok moving forward with what we have here.
app/domain/authentication/authn_oidc/update_input_with_username_from_id_token.rb
Outdated
Show resolved
Hide resolved
b2d52f6
to
0d97396
Compare
end | ||
end | ||
|
||
def token_from_body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestion as above, maybe move to something like:
token_from_body(Hash[URI.decode_www_form(credentials)]
def token_from_body(credential_token)
# Return the parsed body if it include the token
credential_token unless credential_token.fetch('id_token', "").empty?
end
The above change makes the method stateless and should simplify testing the happy and sad paths.
50580ee
to
eb29934
Compare
febba5d
to
0d12713
Compare
service_id_part = service_id ? "/#{service_id}" : "" | ||
path = "#{conjur_hostname}/authn-oidc#{service_id_part}/#{account}/authenticate" | ||
headers = {} | ||
headers["Authorization"] = "Bearer #{id_token}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please use a valid JWT token for the ID token as one would expect for a JWT bearer token? It'll make me feel better to have one test with a base64 encoded value.
If this endpoint takes a JSON object, it's not really following the spirit of using JWT tokens in the header for authorization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind. I just found the JWT generation here:
@oidc_id_token = (JSON.parse(@response_body))["id_token"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Please rebase and cleanup commit history (ideally a single commit).
0d12713
to
2e3c6d4
Compare
5cbb2f4
2e3c6d4
to
5cbb2f4
Compare
5cbb2f4
to
ff46ce5
Compare
Code Climate has analyzed commit ff46ce5 and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 90.0% (-1.5% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
I know this was merged already but there are some unfixed codeclimate issues. Can you please open a new PR to fix that? |
@szh they are false positive |
Desired Outcome
Adding capability to do OIDC authentication when the ID Token is passed in header
Implemented Changes
The changes are done in controller and its helping classes relevant to OIDC
The changes are to handle requests that get ID Token in header instead of body of request
The request that comes without IDToken in body is trying to read Authorization token from header and
checks if it has a "Bearer " prefix. After it the prefix is cut-out and the token after it is passed as an ID Toke
to the existing validation mechanism
Connected Issue/Story
Resolves ONYX-23732
Definition of Done
OIDC authentication for UI is implemented and passed first testing
Changelog
CHANGELOG update
Test coverage
changes, or
Documentation
README
s) were updated in this PRBehavior
Security