-
Notifications
You must be signed in to change notification settings - Fork 25.2k
[SECURITY] Set Auth-scheme preference #33156
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
Conversation
Some browsers (eg. Firefox) behave differently when presented with multiple auth schemes in 'WWW-Authenticate' header. The expected behavior is that browser select the most secure auth scheme before trying others, but Firefox selects the first presented auth scheme and tries the next one's sequentially. As the interpretation is something we do not control, we can at least present the auth schemes in most to least secure order as server's preference. This commit adds the changes to collect and sort the auth schemes presented by most to least secure. We support 'negotiate', 'bearer' and 'basic' schemes and the order is hardcoded with no customization. If there are user requests we will make this configurable. Unit test to verify the `WWW-Authenticate` header values are sorted by server preference as more secure to least secure auth schemes. Testing done with Firefox, Chrome, Internet Explorer 11. Closes#32699
Pinging @elastic/es-security |
@@ -424,4 +439,30 @@ public void testGetFieldFilterSecurityEnabledLicenseNoFLS() throws Exception { | |||
assertNotSame(MapperPlugin.NOOP_FIELD_FILTER, fieldFilter); | |||
assertSame(MapperPlugin.NOOP_FIELD_PREDICATE, fieldFilter.apply(randomAlphaOfLengthBetween(3, 6))); | |||
} | |||
|
|||
public void testDefaultFailureHandlerFailedAuthenticationRespondsWithSortedWWWAuthenticateHeaderValues() 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.
why not put this in DefaultAuthenticationFailureHandlerTests
and then it doesn't need such a long name? Like we can just have testSortsWWWAuthenticateHeaderValues
in that class
Moved the logic to sort auth schemes in the failure handler constructor and the test to failure handler tests.
private void assertWWWAuthenticateWithSchemes(final ElasticsearchSecurityException ese, final String... schemes) { | ||
assertThat(ese.getHeader("WWW-Authenticate").size(), is(schemes.length)); | ||
// Auth scheme are ordered as more secure to least secure | ||
Arrays.sort(schemes, (o1, o2) -> authSchemePriority(o1).compareTo(authSchemePriority(o2))); |
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 make this explicit? ie there could be a bug in authSchemePriority
so we should validate Negotiate first, then bearer, then basic
Hi @jaymode, I have addressed your review comments, please review when you get some time. Thank you. |
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 left one minor comment, otherwise LGTM
final String bearerAuthScheme = "Bearer realm=\"" + XPackField.SECURITY + "\""; | ||
final String negotiateAuthScheme = randomFrom("Negotiate", "Negotiate Ijoijksdk"); | ||
final Map<String, List<String>> failureResponeHeaders = new HashMap<>(); | ||
failureResponeHeaders.put("WWW-Authenticate", Arrays.asList(basicAuthScheme, bearerAuthScheme, negotiateAuthScheme)); |
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 should randomize the order of this list, ie Collections.shuffle(list, random())
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, addressed this.
Some browsers (eg. Firefox) behave differently when presented with multiple auth schemes in 'WWW-Authenticate' header. The expected behavior is that browser select the most secure auth-scheme before trying others, but Firefox selects the first presented auth scheme and tries the next ones sequentially. As the browser interpretation is something that we do not control, we can at least present the auth schemes in most to least secure order as the server's preference. This commit modifies the code to collect and sort the auth schemes presented by most to least secure. The priority of the auth schemes is fixed, the lower number denoting more secure auth-scheme. The current order of schemes based on the ES supported auth-scheme is [Negotiate, Bearer,Basic] and when we add future support for other schemes we will need to update the code. If need be we will make this configuration customizable in future. Unit test to verify the WWW-Authenticate header values are sorted by server preference as more secure to least secure auth schemes. Tested with Firefox, Chrome, Internet Explorer 11. Closes#32699
@bizybot, have we filed a bug against Firefox (assuming there was no existing one)? Can you please share a link? Thanks! |
Thanks! |
Yeah, we decided it probably wasn't worth waiting on an upstream fix. |
Some browsers (eg. Firefox) behave differently when presented with
multiple auth schemes in 'WWW-Authenticate' header. The expected
behavior is that browser select the most secure auth-scheme before
trying others, but Firefox selects the first presented auth scheme and
tries the next ones sequentially. As the browser interpretation is
something that we do not control, we can at least present the auth
schemes in most to least secure order as the server's preference.
This commit modifies the code to collect and sort the auth schemes
presented by most to least secure. The priority of the auth schemes is
fixed, the lower number denoting more secure auth-scheme.
The current order of schemes based on the ES supported auth-scheme is
[
Negotiate
,Bearer
,Basic
] and when we add future support forother schemes we will need to update the code. If need be we will make
this configuration customizable in future.
Unit test to verify the
WWW-Authenticate
header values are sorted byserver preference as more secure to least secure auth schemes.
Tested with Firefox, Chrome, Internet Explorer 11.
Closes#32699