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

pinentry-mac: 0.9.4 -> 1.1.1.1 #176203

Merged
merged 3 commits into from
Aug 25, 2022
Merged

Conversation

midchildan
Copy link
Member

Description of changes

Upgrades pinentry-mac. The upstream repository moved from https://github.com/GPGTools/pinentry-mac to https://github.com/GPGTools/pinentry back in 2018, but seemingly went unnoticed.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

The result of running echo GETPIN | result/Applications/pinentry-mac.app/Contents/MacOS/pinentry-mac:
Screen Shot 2022-06-04 at 4 04 38 PM

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/1095

@SuperSandro2000
Copy link
Member

This shouldn't go to staging but straight to master

@midchildan midchildan changed the base branch from staging to master August 14, 2022 10:14
@midchildan
Copy link
Member Author

I switched the target branch back to master and rebased it against the latest commit.

@SuperSandro2000
Copy link
Member

@ofborg build pinentry_mac

@midchildan
Copy link
Member Author

midchildan commented Aug 16, 2022

/usr/bin/ibtool --compile Main.nib Main.xib
xcode-select: error: no developer tools were found at '/Applications/Xcode.app', and no install could be requested (perhaps no UI is present), please install manually from 'developer.apple.com'.
make[2]: *** [Makefile:741: Main.nib] Error 1

Hmm, so I guess this is unbuildable on Hydra. I can think of 3 options to deal with this problem:

  1. Generate the nib files locally and include them in Nixpkgs.
  2. Set hydraPlatforms to []. Might be problematic because it seems 100 ~ 500 packages depend on pinentry-mac.
  3. Stick with pinentry-mac 0.9.4.

My preference is option 1.

@SuperSandro2000
Copy link
Member

or can we use the tools in nixpkgs?

@midchildan
Copy link
Member Author

There's no complete open source ibtool replacement that I'm aware of. There's xib2nib, but it doesn't work for pinentry_mac:

       > Unhandled attribute:/document/objects/window/point:canvasLocation:x:71
       > Unhandled attribute:/document/objects/window/point:canvasLocation:y:-49
       > Unhandled node:/document/objects/customFormatter:(null)
       > Unhandled attribute:/document/objects/customFormatter:(null):customClass:PassphraseLengthFormatter
       > Unhandled node:/document/resources:(null)
       > Unhandled node:/document/resources/image:(null)
       > Unhandled attribute:/document/resources/image:(null):name:Icon
       > Unhandled attribute:/document/resources/image:(null):width:512
       > Unhandled attribute:/document/resources/image:(null):height:512
       > /nix/store/cm4394z3j2kmcbn3ajhyv2jmb4k3jfmj-stdenv-darwin/setup: line 96: 75124 Segmentation fault: 11  xib2nib Pinentry.xib Pinentry.nib

@midchildan
Copy link
Member Author

I updated this PR with this approach:

Generate the nib files locally and include them in Nixpkgs.

@@ -0,0 +1 @@
root = true
Copy link
Member Author

Choose a reason for hiding this comment

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

This is here to stop editorconfig from throwing errors on binary nib files.

Copy link
Member

Choose a reason for hiding this comment

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

Please move this into the top-level file

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@Luflosi Luflosi left a comment

Choose a reason for hiding this comment

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

LGTM.

@SuperSandro2000
Copy link
Member

I don't really want to check in the binary files into nixpkgs...

@midchildan
Copy link
Member Author

Agreed, but there don't seem to be any other options:

  • Removing pinentry-mac from Hydra is problematic because it's popular and affects too many existing packages in Nixpkgs
  • Holding off updates indefinitely is especially problematic for security-related software

This means we have to generate the nib files somehow, but that's not practically possible without using ibtools from XCode, which in turn can't be done from within the sandbox.

It's not great, but at least in the case of pinentry-mac the binaries are only 70 KB total. Judging by the release history, updates are infrequent too.

@Luflosi
Copy link
Contributor

Luflosi commented Aug 19, 2022

Would it be a good idea to host these files elsewhere and then only store the URL and hash in Nixpkgs?

@midchildan
Copy link
Member Author

midchildan commented Aug 19, 2022

That requires someone to maintain the files independently of Nixpkgs, which would limit who would be able to perform updates. It's doable, but I worry it might lead to updates being hold off for a long time.

@ylh
Copy link
Contributor

ylh commented Aug 22, 2022

Yes, checking in binaries feels a little gross, but it's worth noting that Main.xib and Pinentry.xib have not changed in years, and going by the commit history, they're unlikely to change that often going forward. If by some run of bad luck they do, and the corresponding version is security-critical, it's always possible to mark as insecure while we scramble to generate fresh nibs.

Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

LGTM

Built with nix-shell -p nixpkgs-review --run "nixpkgs-review pr 176203 -p pinentry_mac"

And tested with: echo GETPIN | results/pinentry_mac/Applications/pinentry-mac.app/Contents/MacOS/pinentry-mac

The first time I ran this I got:

bash: cho: command not found            
OK Pleased to meet you

Not sure what that was about but it did not reoccur.

Carrying binaries in Nixpkgs isn't great but until something like xib2nib becomes capable of generating the nibs for this I don't really see a good alternative.

@lovesegfault lovesegfault merged commit 2c9fb7a into NixOS:master Aug 25, 2022
@midchildan midchildan deleted the update/pinentry-mac branch August 25, 2022 13:41
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.

8 participants