-
Notifications
You must be signed in to change notification settings - Fork 505
METRON-1895: Add Knox SSO as an option in Metron #1281
Conversation
| /** | ||
| * Extracts the Knox token from the configured cookie. If basic authentication headers are present, SSO authentication | ||
| * is skipped. | ||
| * @param 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.
Can you quick run through and finish up the javadocs with param and throws descriptions updated?
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.
| final Authentication authentication = new UsernamePasswordAuthenticationToken( | ||
| principal, "", grantedAuths); | ||
| WebAuthenticationDetails webDetails = new WebAuthenticationDetails(httpRequest); | ||
| ((AbstractAuthenticationToken) authentication).setDetails(webDetails); |
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.
You could kill this awkward cast by just leaving Authentication authentication as UsernamePasswordAuthenticationToken authentication, since you're using an implementation level method.
It would just become
final UsernamePasswordAuthenticationToken authentication = new UsernamePasswordAuthenticationToken(
principal, "", grantedAuths);
WebAuthenticationDetails webDetails = new WebAuthenticationDetails(httpRequest);
authentication.setDetails(webDetails);
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.
Good catch. Done.
| * @throws ServletException | ||
| */ | ||
| @Override | ||
| public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) |
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.
Several of the Exceptions throughout the class don't get thrown. Can you drop them from the method sigs?
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 removed the unused exceptions where I could. The exceptions on doFilter are actually thrown so they need to stay.
| protected boolean validateSignature(SignedJWT jwtToken) { | ||
| // Verify the token signature algorithm was as expected | ||
| String receivedSigAlg = jwtToken.getHeader().getAlgorithm().getName(); | ||
| if (!receivedSigAlg.equals("RS256")) { |
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.
Does this need to be hardcoded to this? And can it be pulled into a field/constant somewhere if it does?
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 found a constant in the jwt library. Good suggestion.
| String knoxKey; | ||
| if ((this.knoxKeyString == null || this.knoxKeyString.isEmpty()) && this.knoxKeyFile != null) { | ||
| List<String> keyLines = Files.readAllLines(knoxKeyFile, StandardCharsets.UTF_8); | ||
| if (keyLines != null) { |
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.
Files.readAllLines doesn't return null, so this whole if statement can be simplified.
List<String> keyLines = Files.readAllLines(knoxKeyFile, StandardCharsets.UTF_8);
knoxKey = String.join("", keyLines);
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.
|
|
||
| public static RSAPublicKey parseRSAPublicKey(String pem) | ||
| throws CertificateException, UnsupportedEncodingException { | ||
| String PEM_HEADER = "-----BEGIN CERTIFICATE-----\n"; |
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.
It seems odd that we have to hardcode this. Does this not exist as a library (or at least constants somewhere)?
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 code came straight from the Knox documentation as a code snippet. I moved it to SecurityUtils because I think it makes more sense in a utility class. Is this good enough?
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.
Yeah, I'm fine with that. It's a bit annoying, but if there's not a library to handle this sort of thing, it is what it is.
| PublicKey key = null; | ||
| try { | ||
| CertificateFactory fact = CertificateFactory.getInstance("X.509"); | ||
| ByteArrayInputStream is = new ByteArrayInputStream(fullPem.getBytes("UTF8")); |
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.
fullPem.getBytes("UTF8") to fullPem.getBytes(StandardCharsets.UTF_8)
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.
| .and("member") | ||
| .is(ldapName), (AttributesMapper<String>) attrs -> (String) attrs.get("cn").get()) | ||
| .stream() | ||
| .map(group -> String.format("ROLE_%s", group.toUpperCase())) |
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 there any way to retrieve the "ROLE_" prefix programmatically? It's not a big deal, but if we can avoid hardcoding it, I'd like to.
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 created a ROLE_PREFIX constant and changed everything to use that.
| @EnableSwagger2 | ||
| public class SwaggerConfig { | ||
|
|
||
| @Value("${knox.root}") |
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 the interest of sparking a software religious war, apparently Spring recommends not using field injection for Beans.
I know we do it in multiple places, so I'm not necessarily concerned about fixing it here, but I am interested in your thoughts on that since you're way more familiar with Spring than I am.
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 agree with you. I switched this to use an injected Environment object to get property values. This more closely matches other parts of the REST code.
|
Pluggability came up a few times in the dev discussion. Would you be able to give a brief runthrough of what would be involved in a using choosing to plugin another option? In addition, some of the configs are "knox.sso.". Is there a way to at least generalize some of this for SSO, or does it have to be Knox specific? Or can some of it be what Knox specifically requires, while leaving some of it more general? These seem like they're fairly tied into things like |
|
@justinleet I realized in my testing that the changes in this PR need to be included in #1275 for that PR to work correctly. So am planning on merging these commits into that branch. I want to make sure your comments are addressed so I will add those changes here. Once you're satisfied, I will merge this branch and we can continue the discussion there. |
|
The latest commit should address the inline comments. As for making this feature pluggable, I'm not aware of how other SSO solutions work so I'm not even sure what the abstraction would look like. If we ever did decide to expose another SSO option, I think diving deep into our source code is inevitable. |
|
@merrimanr I'm good with my comments. Go ahead and merge this into the other PR and we'll carry on from there. |
|
This was merged into #1275. Closing. |
Contributor Comments
This PR enables Metron to use Knox SSO for authentication. This PR depends on #1275 and, in it's current state, is meant only as a way to demonstrate this capability.
Testing
Follow the instructions in the "Testing" section of METRON-1878: Add Metron as a Knox service #1275 to add Metron as a Knox service. Once you can access REST and the UIs through Knox you are ready for the next steps.
Add the Metron REST host to the Knox SSO white list. Navigate to the
Advanced knoxsso-topologysetting in Ambari atServices > Knox > Configs > Advanced knoxsso-topology. This property contains the complete Knox SSO topology xml. Update theknoxsso.redirect.whitelist.regexparam value to includenode1. The complete xml should look like this:Save and restart Knox.
metron.xmltopology file at/usr/hdp/current/knox-server/conf/topologies/metron.xmlto use Knox SSO authentication. Replace this:With this:
Copy the key value (on the next line after
Done). Navigate to theMetron Spring optionsproperty in Ambari atServices > Metron > Configs > REST > Metron Spring options. Append the Knox public key property and value to the existingMetron Spring optionsvalue. It should look like:Restart REST. You should now be able to access Metron using Knox SSO.
You should be redirected to the Knox SSO url with the
originalUrlparam properly set:After authenticating with LDAP credentials, you should be successfully redirected and see the global config in the browser.
Navigate to the Alerts UI:
You should go directly to the Alerts UI without being prompted for credentials.
adminuser withadmin/admin-password. Thehttps://node1:8443/gateway/metron/metron-rest/api/v1/user/whoami/rolesendpoint should returnROLE_USERandROLE_ADMINjust as it currently does.Outstanding Issues
This is meant only as a demonstration currently and will likely change depending on the outcome of #1275. Set up is a cumbersome, manual process right now so we will need to make that easier or automate it.
We also need to consider how we manage the
metron.xmltopology file and potentially add our own Knox SSO topology file. We can apply the decisions we make in #1275 for this case as well.I am also planning on adding a comprehensive unit test for the KnoxSSOAuthenticationFilter class and maybe refactoring it down to a couple different classes. Feedback welcome there.
Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
For all changes:
For code changes:
Have you included steps to reproduce the behavior or problem that is being changed or addressed?
Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
For documentation related changes:
Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via
site-book/target/site/index.html:Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that travis-ci is set up for your personal repository such that your branches are built there before submitting a pull request.