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

Make it possible to use a client certificate stored in hardware #246

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jeffallen
Copy link

This has been tested with a Yubikey and the OpenSC PKCS#11 interface on Linux.

This has been tested with a Yubikey and the OpenSC PKCS#11 interface on Linux.
@jeffallen
Copy link
Author

jeffallen commented Mar 28, 2024

Hello @danielgtaylor . This is something we need at Exoscale, and we wanted to show it to you and see what you think. The fact that it pulls in code that needs cgo is unfortunate but more or less unavoidable for how PKCS#11 plugins work.

If that's a deal breaker we'll see what we can do about it. One possibility is to put the pkcs11 stuff into a separate binary (in a separate repo) called "pkcs11-interface", which would talk to restish (and other systems) via i/o on stdio.

I plan to send in some kind of fix for the Windows build failure. The proposed pkcs11-interface would fix Windows as well.

@danielgtaylor
Copy link
Owner

@jeffallen thanks for the PR. I have to admit I'm not familiar with this plugin system and how it works. Given most users install from homebrew or binaries I'm not too worried about adding cgo, but I do like the idea of a plugin similar to how we can currently shell out for custom auth scripts. I'll have to read up on this a bit and figure out how I can test it out.

Copy link

codecov bot commented Mar 30, 2024

Codecov Report

Attention: Patch coverage is 0% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 76.18%. Comparing base (d16bdd7) to head (4875cdb).

❗ Current head 4875cdb differs from pull request most recent head 4c2b845. Consider uploading reports for the commit 4c2b845 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
- Coverage   76.86%   76.18%   -0.69%     
==========================================
  Files          26       26              
  Lines        3679     3712      +33     
==========================================
  Hits         2828     2828              
- Misses        643      675      +32     
- Partials      208      209       +1     
Files Coverage Δ
cli/apiconfig.go 83.87% <ø> (ø)
cli/request.go 60.53% <0.00%> (-7.51%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d16bdd7...4c2b845. Read the comment docs.

This might fix the Windows build error.
@jeffallen
Copy link
Author

FYI, the upgrade to Go 1.22.2 fixed the Windows build error: https://github.com/exoscale/restish/actions/runs/8784557557

@jeffallen jeffallen marked this pull request as ready for review May 14, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants