Skip to content
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

Merged
merged 13 commits into from
Nov 19, 2024

Conversation

brandonstrohmeyer
Copy link
Contributor

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:

  1. A new --mfa-device flag and global option. This flag matches the device name returned from OneLogin.
  2. YubiKey Autodetection. If a YubiKey is connected to the machine executing Clisso, it will automatically be selected as the MFA device to use.

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.

@CLAassistant
Copy link

CLAassistant commented Jan 17, 2024

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Jan 17, 2024

Pull Request Test Coverage Report for Build 11916941105

Details

  • 24 of 75 (32.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 34.917%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cmd/get.go 7 9 77.78%
yubikey/yubikey.go 0 18 0.0%
onelogin/get.go 17 48 35.42%
Totals Coverage Status
Change from base Build 11914010387: 0.2%
Covered Lines: 713
Relevant Lines: 2042

💛 - Coveralls

@bitte-ein-bit
Copy link
Member

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

@bitte-ein-bit
Copy link
Member

@brandonstrohmeyer For the time being, may I suggest to split the two (Yubikey detection, flag for default MFA)?

@bitte-ein-bit
Copy link
Member

@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.

@bitte-ein-bit bitte-ein-bit marked this pull request as ready for review May 23, 2024 05:28
@brandonstrohmeyer
Copy link
Contributor Author

@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:

For the time being, may I suggest to split the two (Yubikey detection, flag for default MFA)?

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.

@bitte-ein-bit
Copy link
Member

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.

onelogin/get.go Show resolved Hide resolved
onelogin/get.go Outdated Show resolved Hide resolved
yubikey/yubikey.go Outdated Show resolved Hide resolved
onelogin/get.go Show resolved Hide resolved
@brandonstrohmeyer
Copy link
Contributor Author

brandonstrohmeyer commented Nov 19, 2024

@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?

@bitte-ein-bit
Copy link
Member

@brandonstrohmeyer Snyk lists some dependency chain issues. But none with a fix available.

@bitte-ein-bit bitte-ein-bit merged commit cd55721 into allcloud-io:master Nov 19, 2024
12 of 13 checks passed
@brandonstrohmeyer brandonstrohmeyer deleted the detect-yubikey branch November 19, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants