-
Notifications
You must be signed in to change notification settings - Fork 314
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
feat(nm): Backend implementation to support EAP-TLS + Minor WebUI fixes #4872
feat(nm): Backend implementation to support EAP-TLS + Minor WebUI fixes #4872
Conversation
The snapshot I used to test this PR: |
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 comments.
Furthermore there are test failures. You'll probably need to update some unit tests to take into account the newly introduced dependency on the KeystoreService and actually test the newly introduced functionality.
...org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMSettingsConverter.java
Outdated
Show resolved
Hide resolved
...org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMSettingsConverter.java
Outdated
Show resolved
Hide resolved
...org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMSettingsConverter.java
Outdated
Show resolved
Hide resolved
...org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMSettingsConverter.java
Outdated
Show resolved
Hide resolved
How I set up EAP-TLS for testing:
|
b2d9382
to
b66808f
Compare
what a working config looks like:
|
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 addition to the comments I provided I need some clarification about the scope of this PR: the title says "initial implementation of tls enterprise backend" but I see changes also in the frontend. Is this complete at this point?
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMSettingsConverter.java
Outdated
Show resolved
Hide resolved
...org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMSettingsConverter.java
Outdated
Show resolved
Hide resolved
...org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMSettingsConverter.java
Outdated
Show resolved
Hide resolved
...se.kura.nm.test/src/test/java/org/eclipse/kura/nm/configuration/NMSettingsConverterTest.java
Outdated
Show resolved
Hide resolved
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
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's still some minor issues... we're almost there!
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...ipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMSettingsConverter.java
Show resolved
Hide resolved
6668ef1
to
4465939
Compare
…eMethod Co-authored-by: Mattia Dal Ben <[email protected]>
c815b62
to
820d627
Compare
@GregoryIvo there's a couple of issues reported by Sonar that require fixing. The code smells are all easy and quick, the coverage needs some more work though. Let me know if you need help with that. |
...org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMSettingsConverter.java
Outdated
Show resolved
Hide resolved
...org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMSettingsConverter.java
Outdated
Show resolved
Hide resolved
...org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/NMSettingsConverter.java
Outdated
Show resolved
Hide resolved
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.
Whenever possible I would avoid committing formatting changes to files you didn't actually change. It adds to the noise and make reviews more difficult.
Obviously, since the formatting changes are correct, leave it as-is but take this into account for the future.
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.
Whenever possible I would avoid committing formatting changes to files you didn't actually change. It adds to the noise and make reviews more difficult.
Obviously, since the formatting changes are correct, leave it as-is but take this into account for the future.
if (modifiedProps.containsKey(key)) { | ||
|
||
Object prop = modifiedProps.get(key); | ||
|
||
if (prop instanceof String) { | ||
String keystorePid = (String) prop; | ||
|
||
findAndDecodeCertificatesForInterface(interfaceName, modifiedProps, | ||
this.keystoreServices.get(keystorePid)); | ||
} | ||
} |
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 can leave the code as-is but consider using Early Returns in the future. I think the code is much easier to read:
if (modifiedProps.containsKey(key)) { | |
Object prop = modifiedProps.get(key); | |
if (prop instanceof String) { | |
String keystorePid = (String) prop; | |
findAndDecodeCertificatesForInterface(interfaceName, modifiedProps, | |
this.keystoreServices.get(keystorePid)); | |
} | |
} | |
if (!modifiedProps.containsKey(key)) { | |
return; // we cannot "continue" since we're inside a lambda | |
} | |
Object keystorePid = modifiedProps.get(key); | |
if (!keystorePid instanceof String) { | |
return; | |
} | |
findAndDecodeCertificatesForInterface(interfaceName, modifiedProps, | |
this.keystoreServices.get((String) keystorePid)); |
@GregoryIvo I think the code is OK. I suggested a small change to avoid magic numbers in the code and have a couple of questions ([1], [2] and [3]). Everything else I mentioned is optional and is not required for merging. I will now move on with testing. |
@@ -295,6 +315,86 @@ private void decryptAndConvertPasswordProperties(Map<String, Object> modifiedPro | |||
} | |||
} | |||
|
|||
private void decryptAndConvertCertificatesProperties(Map<String, Object> modifiedProps, Set<String> interfaces) { |
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.
One final thought: this is a Side Effect Method.
A side effect method is a method which modifies some state variable value/arguments passed having a consequence beyond its scope, that is to say it has an observable effect besides returning a value (the main effect) to the invoker of the operation.
which is a known anti-pattern/bad practice (see Uncle Bob Martin's "Clean Code", there's a dedicated section for that).
The correct way to handle this would be to return a modified version of the passed argument, but we already had the decryptAndConvertPasswordProperties
that behaved the same way and therefore we're doing the same for consistency.
Again: leave it as-is but be aware of the fact that this is not the best implementation.
Also notice that you've hidden the actual modification of the map down a couple of layer of abstractions (you actually change the values inside findAndDecodeCertificatesForInterface
), so you've hidden the Side Effect even more than what was done in decryptAndConvertPasswordProperties
.
Regarding why this is considered a bad practice... you experienced it first hand while chasing that bug caused by the modified map 😆
@GregoryIvo tested on my setup. It works! 🥳 I used the UI in #4881 and the instruction here and here and was able to connect to my WPA Enterprise hotspot. Here's some screenshots of the setup The resulting NM configuration: Expand
Additional notes:
Where:
ran the commands container in the
|
…es (#4872) * feat: initial implimentation of tls backend * fix: weird formatter oddity * fix: removed loggers * refactor: ifPresent if's changed to lamdas * tests: update for new tls changes * fix: revert un-needed change * feat: added support for multiple keystores * tests: removed not needed logging * fix: improved backend stability, and fixed regex * refactor: changed the way the keystore var is passed * feat: added more type security in webUI * refactor: fix comments enable -> unable * refactor: removed to string * refactor: certificate replacement method * refactor: removed not needed Exception * refactor: decryptAndConvertCertificates method * refactor: removed extra newline Co-authored-by: Mattia Dal Ben <[email protected]> * refactor: error message * refactor: added more if checks to improve reliability when applying from webUI * refactor: created a hard copy of the modifiedMap so it is not passed to the configuration service and applied * refactor: removed extra 802-1x in String Co-authored-by: Mattia Dal Ben <[email protected]> * refactor: add specificity to to isCertificate method Co-authored-by: Mattia Dal Ben <[email protected]> * refactor: updated String for better readability Co-authored-by: Mattia Dal Ben <[email protected]> * refactor: add extra safety checks in getTrustedCertificateFromKeystoreMethod Co-authored-by: Mattia Dal Ben <[email protected]> * lint: fix whitespace issues * fix: changed 802-1x -> 802.1x * refactor: removed unnecessary cast to String * refactor: removed extra curly braces in lamdas * refactor: removed type in <> * tests: added basic enterprise test coverage * test: added method for mocking keystore * fix: added static variable for NM_SECRET_FLAGS_NOT_REQUIRED * fix: removed generic exception logging, and changed exceptions --------- Co-authored-by: Mattia Dal Ben <[email protected]>
…es (#4872) * feat: initial implimentation of tls backend * fix: weird formatter oddity * fix: removed loggers * refactor: ifPresent if's changed to lamdas * tests: update for new tls changes * fix: revert un-needed change * feat: added support for multiple keystores * tests: removed not needed logging * fix: improved backend stability, and fixed regex * refactor: changed the way the keystore var is passed * feat: added more type security in webUI * refactor: fix comments enable -> unable * refactor: removed to string * refactor: certificate replacement method * refactor: removed not needed Exception * refactor: decryptAndConvertCertificates method * refactor: removed extra newline Co-authored-by: Mattia Dal Ben <[email protected]> * refactor: error message * refactor: added more if checks to improve reliability when applying from webUI * refactor: created a hard copy of the modifiedMap so it is not passed to the configuration service and applied * refactor: removed extra 802-1x in String Co-authored-by: Mattia Dal Ben <[email protected]> * refactor: add specificity to to isCertificate method Co-authored-by: Mattia Dal Ben <[email protected]> * refactor: updated String for better readability Co-authored-by: Mattia Dal Ben <[email protected]> * refactor: add extra safety checks in getTrustedCertificateFromKeystoreMethod Co-authored-by: Mattia Dal Ben <[email protected]> * lint: fix whitespace issues * fix: changed 802-1x -> 802.1x * refactor: removed unnecessary cast to String * refactor: removed extra curly braces in lamdas * refactor: removed type in <> * tests: added basic enterprise test coverage * test: added method for mocking keystore * fix: added static variable for NM_SECRET_FLAGS_NOT_REQUIRED * fix: removed generic exception logging, and changed exceptions --------- Co-authored-by: Mattia Dal Ben <[email protected]>
This PR adds some basic support for TLS.
add your certs to the KeystoreManager,
point the keystone, and cert names to the NM service
certs are decoded passed to NM, and a config is established.
Note: there may be a possible race condition with NM and the keystore service, which needs to be investigated.
Brief description of the PR. [e.g. Added
null
check onobject
to avoidNullPointerException
]Related Issue: This PR fixes/closes {issue number}
Description of the solution adopted: A more detailed description of the changes made to solve/close one or more issues. If the PR is simple and easy to understand this section can be skipped
Screenshots: If applicable, add screenshots to help explain your solution
Manual Tests: Optional description of the tests performed to check correct functioning of changes, useful for an efficient review
Any side note on the changes made: Description of any other change that has been made, which is not directly linked to the issue resolution [e.g. Code clean up/Sonar issue resolution]