Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Add Yubico Universal Two-Factor Libraries and U2F PAM Module #43676

Closed
wants to merge 3 commits into from

Conversation

dlo
Copy link
Contributor

@dlo dlo commented Sep 7, 2015

Contains three formula:

  • libu2f-server
  • libu2f-host
  • pam_u2f (requires above two)

Thanks!

depends_on "json-c" => :build

def install
args = ["--prefix=#{prefix}"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's just one arg inline it. system "./configure", "--prefix=#{prefix}"

Thanks

@justinmayer
Copy link
Contributor

Thanks for your work on this, Dan. Excited for it to land in Homebrew! 🎉

@dlo
Copy link
Contributor Author

dlo commented Sep 12, 2015

Thanks Justin!

@dlo
Copy link
Contributor Author

dlo commented Sep 12, 2015

@MikeMcQuaid Unfortunately, the PAM module itself (pam_u2f) cannot be tested without a human--the only way to ensure installation has succeeded is to have a physical U2F USB key and update your PAM configuration accordingly. The best I can think of is to copy the format set by pam_yubico (https://github.com/Homebrew/homebrew/blob/f2eada5f48597e0d688aaa36907a8ba29bf62f40/Library/Formula/pam_yubico.rb#L36)

However, I was able to add tests for libu2f-host and libu2f-server. Let me know if there's anything else here that needs to be addressed. :)

@dlo
Copy link
Contributor Author

dlo commented Sep 13, 2015

All tests now passing.

@dlo dlo changed the title Add Yubico Universal Two-Factor Libraries and PAM Module Add Yubico Universal Two-Factor Libraries and U2F PAM Module Sep 13, 2015
@dlo
Copy link
Contributor Author

dlo commented Sep 15, 2015

@MikeMcQuaid @DomT4 Any more updates needed here? Would love to get this merged in!

def caveats; <<-EOS.undent
To use the U2F for PAM authentication, specify the full path to the module (#{lib}pam/pam_u2f.so) in a PAM configuration. You can find all PAM configurations in /etc/pam.d.

For further installation instructions, please visit https://developers.yubico.com/pam-u2f/#installation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you ensure that these lines are wrapped at 80 characters, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dlo
Copy link
Contributor Author

dlo commented Sep 26, 2015

@MikeMcQuaid Should be good to go here. :)

@bfontaine
Copy link
Contributor

Ping on the pam-u2f release?

@dlo
Copy link
Contributor Author

dlo commented Dec 16, 2015

Checking out comments now...thought I had finished up things on my end but new things came up.

@dlo dlo force-pushed the u2f branch 2 times, most recently from 805ea73 to 309cae7 Compare December 16, 2015 18:28
@dlo
Copy link
Contributor Author

dlo commented Dec 16, 2015

@bfontaine @DomT4 @MikeMcQuaid just updated to use upstream release

desc "Provides an easy way to use U2F-compliant authenticators with PAM."
homepage "https://developers.yubico.com/pam-u2f/"
url "https://github.com/Yubico/pam-u2f/archive/pam_u2f-1.0.3.tar.gz"
version "1.0.3"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this line; Homebrew extracts the version from the URL.

@dlo
Copy link
Contributor Author

dlo commented Dec 16, 2015

@bfontaine @DomT4 @MikeMcQuaid Can we please cap the rounds of comments please? I have no problem making this last round of changes but at this point I'm getting feedback on things that could have been mentioned months ago. The seemingly neverending back and forth is making this tiresome for me.

Can we agree that after this round is complete, we're good to go?

@bfontaine
Copy link
Contributor

@dlo These two style comments should be the last ones; if you check the build everything went fine except for the audit part: https://travis-ci.org/Homebrew/homebrew/jobs/97281456

@MikeMcQuaid
Copy link
Member

@dlo Yup, should be good to ship after that. I'm sorry that it's seemed never-ending but a bunch of these things are flagged for you by tools you can run locally (brew audit --strict --online)

@bfontaine
Copy link
Contributor

Ping?

@dlo
Copy link
Contributor Author

dlo commented Jan 26, 2016

@bfontaine Thanks for the reminder--totally got lost on my radar. Just updated with changes. Thanks!

@dlo
Copy link
Contributor Author

dlo commented Jan 27, 2016

@bfontaine Alright, tests are green and all comments addressed. :)

@DomT4
Copy link
Member

DomT4 commented Jan 28, 2016

Merged in f2a262a, 7e524fc and 4ea6657.

Thanks for sticking with us during the review, and thank you for your contributions to Homebrew @dlo! We appreciate them 😺

@DomT4 DomT4 removed the CI-requeued label Jan 28, 2016
@DomT4 DomT4 closed this in 4ea6657 Jan 28, 2016
@dlo
Copy link
Contributor Author

dlo commented Jan 28, 2016

@DomT4 @bfontaine @MikeMcQuaid Yay! Thanks all. Glad to see this finally merged. :)

@bfontaine
Copy link
Contributor

🎉

@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants