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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

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
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ bitte-ein-bit
❌ brandonstrohmeyer
You have signed the CLA already but the status is still pending? Let us recheck it.

@coveralls
Copy link

coveralls commented Jan 17, 2024

Pull Request Test Coverage Report for Build 9172670778

Details

  • 23 of 76 (30.26%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.5%) to 28.449%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cmd/get.go 8 10 80.0%
yubikey/yubikey.go 0 19 0.0%
onelogin/get.go 15 47 31.91%
Totals Coverage Status
Change from base Build 9171371870: 0.5%
Covered Lines: 497
Relevant Lines: 1747

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

for yubikey support we require CGO support. CGO on crosscompile is hard,
to avoid we build natively where suppported and disabled where not

since goreleaser does only support prebuilt archives in the pro version,
the brew publishing step had to be moved to a different github action
@bitte-ein-bit bitte-ein-bit marked this pull request as ready for review May 23, 2024 05:28
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