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

[ELY-201] Add client side HTTP authentication mechanism support #528

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

keshav-redhat
Copy link

Copy link
Contributor

@Skyllarr Skyllarr left a comment

Choose a reason for hiding this comment

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

Thank you @keshav-redhat ! I added some minor comments

=== Testing By
// Put an x in the relevant field to indicate if testing will be done by Engineering or QE.
// Discuss with QE during the Kickoff state to decide this
* [ ] Engineering
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be tested by engineering AFAIK


* [ ] QE

=== Affected Projects or Components
Copy link
Contributor

Choose a reason for hiding this comment

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

affected projects or components are wildfly-elytron and wildfly


=== Hard Requirements

Having a client which can read the elytron configurations and add authorization header with BASIC,BEARER or DIGEST based on challenge it received.
Copy link
Contributor

Choose a reason for hiding this comment

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

please add using an SSLContext from the elytron configuration tot his list

Having a client which can read the elytron configurations and add authorization header with BASIC,BEARER or DIGEST based on challenge it received.

=== Nice-to-Have Requirements

Copy link
Contributor

Choose a reason for hiding this comment

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

You can put http-selector in the nice to have requirements section

=== Non-Requirements

== Backwards Compatibility

Copy link
Contributor

Choose a reason for hiding this comment

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

You can write here that it does not affect backwards compatibility because we did not provide any HTTP client before

Now after sending the request we will get a response which is of type HttpResponse.we will determine using the status if the authentication success or failed.If status code is 200 then we will know that authentication is successful.

If the authentication failure then headers will contain : 
www-authenticate=[Basic realm="RealmUsersRoles"] for basic authentication.similar for the other authentication mechanism type like digest,bearer,etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not clear what happens after we receive authentication failure. If the authentication results in failure then we create another request created from the needed information and send this new request to the same URI.

Also what is failure in this context? If we receive response code 500 then it does not make sense to try to add credentials? Can you write here whether by authentication failure do you mean codes 401 or 403 or other?

The actual communication flow and connect method have be tested with arquillian in wildfly where we will call the connect method with required parameter and test it.

== Community Documentation
////
Copy link
Contributor

Choose a reason for hiding this comment

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

Community documentation has to be added to the wildfly repository under docs older

Copy link
Author

Choose a reason for hiding this comment

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

@Skyllarr ,I did not get this comment.

Copy link
Contributor

@Skyllarr Skyllarr Jul 5, 2023

Choose a reason for hiding this comment

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

@keshav-redhat new features are being documented in the upstream wildfly documentation that is located in the docs folder

////
== Release Note Content
////
Draft verbiage for up to a few sentences on the feature for inclusion in the
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to draft here a release note content. That should include a brief description of this feature

== Test Plan

To test the above functionality we can add the tests as follows : 
Inside tests/base/src/test/java/org/wildfly/security/http/client/ we will create a test class named as ElytronHttpClientTest where we will get an instance of HttpRequest from ElytronHttpClient#evaluateMechanism() method of different authentication mechanism and then we will create an object of TestingHttpServerRequest using request header which we will pass to the evaluateRequest() method of BasicAuthenticationMechanism which is used to evaluate our header values of request method.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am having difficulties understanding this long sentence. Can you please reformulate this? Also by object of TestingHttpServerRequest you mean instance of right? since the class already exists. Also it is not clear when you are refering to the authentication mechanism of the client and when you are refering to the authentication mechanism of the server


== Overview

Wildfly Elytron already provide client and server side SASL mechanism for server side HTTP authentication mechanism.This will include a client side framework for HTTP authentication mechanisms and implement a set of client side HTTP authentication mechanisms.
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@jamezp we did not discuss the Authenticator usage. But it seems that for our use case where we need to obtain not just a password from the elytron configuration but also the ssl context, credentials stored in the credential store, and bearer tokens it is not so clear how the Authenticator would fit in. Let us know if you have any idea or a suggestion for it

Copy link
Member

Choose a reason for hiding this comment

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

@Skyllarr
Copy link
Contributor

Skyllarr commented Jul 5, 2023

@keshav-redhat Just a minor, if you'll have time, you can consider and add to this proposal the ideas you had about how to make this extensible for other clients than java HTTP client, like apache HTTP client for example. Thank you!

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

3 participants