-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add default MFA device selection for OneLogin #367
Add default MFA device selection for OneLogin #367
Conversation
Pull Request Test Coverage Report for Build 11916941105Details
💛 - Coveralls |
Hey @brandonstrohmeyer, thank you for this PR! Please don't forget to accept the Contributer License Agreement. Goreleaser is a little tricky when it comes to CGO. Bytebase did a pretty awesome walk through. I fear we have to go a similar approach. Unfortunately, that also means I have to move the MacOS notarization to Github actions. That's going to be a little bigger pain. If you would be so kind to follow the bytebase write-up, then I can take care of the notarization later. Thanks |
@brandonstrohmeyer For the time being, may I suggest to split the two (Yubikey detection, flag for default MFA)? |
b9fa7b2
to
3ee6f2f
Compare
@brandonstrohmeyer I've reworked the build system. Can you test that I didn't mess up while merging the changes from the master branch? I don't own a Yubikey to test. Please also sign the Contributor Agreement. |
3ee6f2f
to
dc8bd3b
Compare
@bitte-ein-bit apologies for going MIA, things got busy and side projects fell by the wayside...It really doesn't feel like I opened this PR nearly a year ago. I have signed the CLA. I have tested all the changes you've made, and after building the macos binary locally and signing it with a self-signed cert everything was still working as expected. I see your previous comment:
Do you still want me to move yubikey detection to its own PR? No trouble if so, I'm just trying to get reacquainted with this project. |
Hey @brandonstrohmeyer, Yeah, time flies very fast. No, there is no reason to split the two items anymore. I wanted to move forward faster without completely reworking the build system. But that's out of the way now, so we should be good. Unfortunately, I couldn't fully get CGO setup on some OS/Archs combinations. Major x86 versions should be covered. Please add some remarks in the readme. Let's fix the conflicts, get it tested and merged. I've left some comments in the code. |
299f5fe
to
7b8b86e
Compare
@bitte-ein-bit I've fixed the conflicts and made some corrections based on your feedback. Thank you very much for getting the CGO stuff sorted, I've made notes in the readme for which platforms it (and thus Yubikey autodetection) supports. I see the security/snyk test is failing but don't have access to see what exactly the issue is. Are you able to provide some detail? |
@brandonstrohmeyer Snyk lists some dependency chain issues. But none with a fix available. |
This PR adds support for a default MFA device with OneLogin, saving the user the trouble of the single keystroke to select their MFA device from a list.
There are two primary features:
--mfa-device
flag and global option. This flag matches the device name returned from OneLogin.The YubiKey autodetection relies on the karalabe/hid package which needs CGO enabled and appropriate C toolchain installed in order to cross compile. I did not make these changes to .goreleaser as I'm not sure exactly how the build machine is setup and what toolchains are available to use. I'm happy to make any requested changes, but am asking for a little guidance here. Opening as a Draft until the .goreleaser issue is resolved.