-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Feature/Identity] Adds Basic Auth mechanism via Internal IdP #4798
[Feature/Identity] Adds Basic Auth mechanism via Internal IdP #4798
Conversation
Gradle Check (Jenkins) Run Completed with:
|
5d3277f
to
42e8115
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
A whole bunch of tests are failing because of addition of REST request wrapper (literally a breaking change 😅). Looking into possible solutions to fix those now. |
@JsonProperty(value = "primary_principal") | ||
public StringPrincipal getPrimaryPrincipal() { | ||
public StringPrincipal getPrincipal() { |
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 previous version implied that a subject could have more than one principal (which is true), this one does not imply this. Is this intentional or was this change made since it should only ever be interacting with a specific principal at a time?
@JsonProperty(value = "primary_principal") | ||
public StringPrincipal getPrimaryPrincipal() { | ||
public StringPrincipal getPrincipal() { | ||
return primaryPrincipal; |
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.
May want to rename this to be consistent if you are renaming the function call.
} else if (!isAuthenticationSuccessful) { | ||
throw new AuthenticationException("Authentication finally failed"); | ||
} | ||
} catch (Throwable e) { |
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.
Is this a dead throw?
* @return true if authentication was successful, false otherwise | ||
* @throws Exception generic exception rethrown in case authenticate method throws an exception | ||
*/ | ||
private boolean checkAndAuthenticateRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { |
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.
Consider splitting this into separate formatting and a authenticated checks--may be easier to provide meaningful exception messages.
* @return true if authentication was successful, false otherwise | ||
* @throws IOException when an exception is raised writing response to channel | ||
*/ | ||
private boolean authenticate(RestRequest request, RestChannel channel, NodeClient client) throws IOException { |
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.
See above comment; this method authenticates from within the checkAndAuthenticateRequest -- why not split the two if you already have the separate authenticate method?
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
ce25681
to
61f658d
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle check failing due to unrelated issue. Check logs from this build and log from CI build a day ago Looks like an issue with openJDK installation |
Local run of `./gradlew :server:assemble` is successful:
|
61f658d
to
080d2be
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Seeing
https://netty.io/4.1/api/io/netty/handler/codec/http/TooLongHttpLineException.html Possibly related: elastic/elasticsearch#1174 |
try { | ||
// support other type of header tokens | ||
headerToken = new HttpHeaderToken(authHeader.get()); | ||
subject = Identity.getAuthManager().getSubject(); |
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've seen this call in Shiro documentation, but I'm not quite following how this resolves to the correct subject here.
According to Shiro docs:
In almost all environments, you can obtain the currently executing Subject by using org.apache.shiro.SecurityUtils:
Subject currentUser = SecurityUtils.getSubject();
The getSubject() call in a standalone application might return a Subject based on user data in an application-specific location, and in a server environment (e.g. web app), it acquires the Subject based on user data associated with current thread or incoming request.
https://shiro.apache.org/subject.html
Will this always resolve to the correct Subject?
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.. Shiro has its own resource store and it fetches correct subject via ThreadContext.
This is how shiro puts resource into its ThreadContext store:
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.
According to shiro source, the DefaultSecurityManager uses ThreadContext under the hood: https://github.com/apache/shiro/blob/main/core/src/main/java/org/apache/shiro/SecurityUtils.java#LL54C9-L54C54
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 call to get subject might have to do some weird stuff such as inspecting the call stack or thread context, but the pattern should be abstracted for use cases like this. Does that seem viable to you, or is there a way you think we should consider reframing the calls?
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'm not sure I understand the ask here
Signed-off-by: Craig Perkins <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Darshit Chanpura <[email protected]>
7b7fb72
to
c14ab0c
Compare
Gradle Check (Jenkins) Run Completed with:
|
@cwperks @peternied Can you please review and merge this is there is nothing else? |
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.
There are many build errors, please revert the broad chances to the namespace under server/src/... until these are resolved
@@ -18,7 +18,7 @@ dependencies { | |||
implementation "com.fasterxml.jackson.core:jackson-annotations:${versions.jackson}" | |||
implementation "com.fasterxml.jackson.core:jackson-databind:${versions.jackson_databind}" | |||
|
|||
implementation 'org.apache.shiro:shiro-core:1.9.1' | |||
api 'org.apache.shiro:shiro-core:1.9.1' |
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 don't think this change is needed, Could you provide the why of switching from implementation
to api
dependency type?
// Malformed AuthHeader strings | ||
if (decodedUserNamePassword.length != 2) return null; | ||
|
||
logger.info("Logging in as: " + decodedUserNamePassword[0]); |
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.
:
is accepted in the password according to this comment in the security plugin codebase: https://github.com/opensearch-project/security/blob/cad7c7201e902aeb711b407648d41d3c55fe7a49/src/main/java/org/opensearch/security/support/HTTPHelper.java#L52-L57
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.
What do you think about creating an issue and finding/implementing a better version of this?
🤞 I would rather pull a small library in for this kind of logic
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 will modify the logic to be similar to one that Craig posted above. However, this seems like a good avenue to explore libraries that provide support for this.
@DarshitChanpura Thank you for adding tests. Can you please answer @peternied's question on why the change from I am ok with accepting and merging this change and addressing some of the other comments as follow-ups. From what I can see this is what is remaining:
As far as I can see, there is nothing preventing us from applying multiple wrappers. @peternied @DarshitChanpura What do you think about creating a wrapper in the
and we can handle the rest filtering there as its done in the plug-in model? I'm not positive if the location of the |
Signed-off-by: Darshit Chanpura <[email protected]>
I did. Check this out: #4798 (comment)
I will look into this as a follow-up to this PR.
Done. I have modified
I like this idea. I have an abandoned branch where I experimented with a "filter" for rest requests. It needs some modifications but is ready to go. Check this out: https://github.com/DarshitChanpura/OpenSearch/blob/basic-auth-with-security-rest-filter/server/src/main/java/org/opensearch/rest/SecurityRestFilter.java
I'm positive that this is in the correct place, as |
Gradle Check (Jenkins) Run Completed with:
|
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.
Thank you @DarshitChanpura! This is a good step forward for security in core.
…ests with no headers to pass-through for time being Signed-off-by: Darshit Chanpura <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
… and adds a sample user in example users yml Signed-off-by: Darshit Chanpura <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Darshit Chanpura <[email protected]>
This seems like it is in a good spot. Once this is merged, I will take the bearer auth stuff, and close the draft I have open to make a new PR to introduce bearer auth on top of this. That will give us bearer & basic auth and then we can go from there. |
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Darshit Chanpura <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
DC I know there are failures, I'm going to merge this change as is and you can keep working on them separately. |
Description
Adds a REST request wrapper to intercept all incoming REST requests and authenticate them via Internal IdP (HTTP only for this PR)
NOTE: This PR does not reject the request if no credentials were present and will proceed as normal. This is intentional.
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.