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

Support windows via winscard #56

Closed
tobiaskohlbau opened this issue Apr 29, 2020 · 4 comments · Fixed by #57
Closed

Support windows via winscard #56

tobiaskohlbau opened this issue Apr 29, 2020 · 4 comments · Fixed by #57

Comments

@tobiaskohlbau
Copy link
Contributor

Inspired by the last stream of @FiloSottile I've digged into piv-go and windows and created a port for windows. Are the maintainers willing to merge windows patches into this repo? If it's fine I would put together a PR and would be willing to work with everyone involved.

Thanks in advance and thanks for this nice library.

@ericchiang
Copy link
Collaborator

Generally I'd be open to it, but I don't have any way of testing changes. Right now I manually test PRs on MacOS and Linux machines, so it's likely that Windows support would break over time.

What do the changes look like? Is is just additional cgo configuration here?

piv-go/piv/pcsc.go

Lines 21 to 25 in e1789b3

// #cgo darwin LDFLAGS: -framework PCSC
// #cgo linux CFLAGS: -I/usr/include/PCSC
// #cgo linux LDFLAGS: -lpcsclite
// #include <PCSC/winscard.h>
// #include <PCSC/wintypes.h>

@tobiaskohlbau
Copy link
Contributor Author

tobiaskohlbau commented May 1, 2020

Maybe it's okay to flag the windows support as experimental. We could state the fact that you're unable to test within the readme? Maybe something like Windows is community supported and not field tested by the maintainer.

I've cleaned up the changes and pushed them to https://github.com/tobiaskohlbau/piv-go/tree/feature/addWindowsSupport . I took the road without CGO as it's quiet a hurdle to setup a gcc on windows. The integration was surprisingly nice and conformant with the existing codebase. I did not run the test as I'm not aware which parts of the YK gets wiped and I'm affright of loosing my data on my YK. Best option is to go with another not critical YK... I need to order one.

Edit:

I've just checked the docs and learned that PIV is not overlapping with OpenPGP applet. I did run the tests and everything works.

Logs
=== RUN   TestYubiKeySignECDSA
--- PASS: TestYubiKeySignECDSA (0.24s)
=== RUN   TestPINPrompt
=== RUN   TestPINPrompt/Never
=== RUN   TestPINPrompt/Once
=== RUN   TestPINPrompt/Always
--- PASS: TestPINPrompt (1.35s)
    --- PASS: TestPINPrompt/Never (0.44s)
    --- PASS: TestPINPrompt/Once (0.45s)
    --- PASS: TestPINPrompt/Always (0.46s)
=== RUN   TestSlots
=== RUN   TestSlots/Authentication
=== RUN   TestSlots/CardAuthentication
=== RUN   TestSlots/KeyManagement
=== RUN   TestSlots/Signature
--- PASS: TestSlots (3.89s)
    --- PASS: TestSlots/Authentication (0.58s)
    --- PASS: TestSlots/CardAuthentication (0.57s)
    --- PASS: TestSlots/KeyManagement (0.56s)
    --- PASS: TestSlots/Signature (0.55s)
=== RUN   TestYubiKeySignRSA
=== RUN   TestYubiKeySignRSA/rsa1024
=== RUN   TestYubiKeySignRSA/rsa2048
--- PASS: TestYubiKeySignRSA (3.97s)
    --- PASS: TestYubiKeySignRSA/rsa1024 (1.06s)
    --- PASS: TestYubiKeySignRSA/rsa2048 (2.91s)
=== RUN   TestYubiKeyDecryptRSA
=== RUN   TestYubiKeyDecryptRSA/rsa1024
=== RUN   TestYubiKeyDecryptRSA/rsa2048
--- PASS: TestYubiKeyDecryptRSA (8.02s)
    --- PASS: TestYubiKeyDecryptRSA/rsa1024 (0.87s)
    --- PASS: TestYubiKeyDecryptRSA/rsa2048 (7.15s)
=== RUN   TestYubiKeyAttestation
--- PASS: TestYubiKeyAttestation (0.31s)
=== RUN   TestYubiKeyStoreCertificate
--- PASS: TestYubiKeyStoreCertificate (0.27s)
=== RUN   TestYubiKeyGenerateKey
=== RUN   TestYubiKeyGenerateKey/ec_256
=== RUN   TestYubiKeyGenerateKey/ec_384
=== RUN   TestYubiKeyGenerateKey/rsa_1024
=== RUN   TestYubiKeyGenerateKey/rsa_2048
--- PASS: TestYubiKeyGenerateKey (5.22s)
    --- PASS: TestYubiKeyGenerateKey/ec_256 (0.14s)
    --- PASS: TestYubiKeyGenerateKey/ec_384 (0.19s)
    --- PASS: TestYubiKeyGenerateKey/rsa_1024 (0.93s)
    --- PASS: TestYubiKeyGenerateKey/rsa_2048 (3.96s)
=== RUN   TestYubiKeyPrivateKey
--- PASS: TestYubiKeyPrivateKey (0.37s)
=== RUN   TestYubiKeyPrivateKeyPINError
--- PASS: TestYubiKeyPrivateKeyPINError (0.29s)
=== RUN   TestContextClose
--- PASS: TestContextClose (0.00s)
=== RUN   TestContextListReaders
--- PASS: TestContextListReaders (0.00s)
=== RUN   TestHandle
--- PASS: TestHandle (0.00s)
=== RUN   TestTransaction
--- PASS: TestTransaction (0.00s)
=== RUN   TestErrors
--- PASS: TestErrors (0.00s)
=== RUN   TestGetVersion
--- PASS: TestGetVersion (0.01s)
=== RUN   TestCards
--- PASS: TestCards (0.00s)
=== RUN   TestNewYubiKey
--- PASS: TestNewYubiKey (0.01s)
=== RUN   TestMultipleConnections
--- PASS: TestMultipleConnections (0.01s)
=== RUN   TestYubiKeySerial
--- PASS: TestYubiKeySerial (0.02s)
=== RUN   TestYubiKeyLoginNeeded
--- PASS: TestYubiKeyLoginNeeded (0.02s)
=== RUN   TestYubiKeyPINRetries
--- PASS: TestYubiKeyPINRetries (0.01s)
=== RUN   TestYubiKeyReset
--- PASS: TestYubiKeyReset (1.61s)
=== RUN   TestYubiKeyLogin
--- PASS: TestYubiKeyLogin (0.02s)
=== RUN   TestYubiKeyAuthenticate
--- PASS: TestYubiKeyAuthenticate (0.01s)
=== RUN   TestYubiKeySetManagementKey
--- PASS: TestYubiKeySetManagementKey (0.03s)
=== RUN   TestYubiKeyUnblockPIN
--- PASS: TestYubiKeyUnblockPIN (0.05s)
=== RUN   TestYubiKeyChangePIN
--- PASS: TestYubiKeyChangePIN (0.04s)
=== RUN   TestYubiKeyChangePUK
--- PASS: TestYubiKeyChangePUK (0.04s)
=== RUN   TestChangeManagementKey
--- PASS: TestChangeManagementKey (0.03s)
=== RUN   TestMetadata
--- PASS: TestMetadata (1.60s)
=== RUN   TestMetadataUnmarshal
--- PASS: TestMetadataUnmarshal (0.00s)
=== RUN   TestMetadataMarshal
--- PASS: TestMetadataMarshal (0.00s)
=== RUN   TestMetadataUpdate
--- PASS: TestMetadataUpdate (0.00s)
=== RUN   TestMetadataAdditoinalFields
--- PASS: TestMetadataAdditoinalFields (0.00s)
PASS
ok      github.com/go-piv/piv-go/piv    27.721s

@ericchiang
Copy link
Collaborator

🙌 yeah that patch looks clean. Awesome to see that someone else got the tests to pass too! I wonder if it'd be easy to add a CI builder to ensure it at least compiles (I can also do that separately if you like):

https://github.com/go-piv/piv-go/blob/master/.github/workflows/test.yaml

Go ahead and send a PR. Does something like this in the README sound reasonable?

Windows support is best effort due to lack of test hardware. This means the maintainers will take patches for Windows, but if you encounter a bug or the build is broken, you may be asked to fix it.

@tobiaskohlbau
Copy link
Contributor Author

I've updated the README and put in the github action workflow. I've done it from the back of my mind. Could be that something is broken but I saw that the builders for external PRs are enabled.

Earlier today I thought if it's possible to leverage a raspberry pi with an older yubikey to be a full test suite via custom github actions runner. Would be a cool weekend project to try this out.

tobiaskohlbau added a commit to tobiaskohlbau/piv-go that referenced this issue May 1, 2020
Adds support for windows via winscard. This CL leverages the
winscard api via calling into the dll. For more information
see https://docs.microsoft.com/en-us/windows/win32/api/winscard/.

Fixes go-piv#56

Signed-off-by: Tobias Kohlbau <tobias@kohlbau.de>
tobiaskohlbau added a commit to tobiaskohlbau/piv-go that referenced this issue May 2, 2020
Adds support for windows via winscard. This CL leverages the
winscard api via calling into the dll. For more information
see https://docs.microsoft.com/en-us/windows/win32/api/winscard/.

Fixes go-piv#56

Signed-off-by: Tobias Kohlbau <tobias@kohlbau.de>
ericchiang pushed a commit that referenced this issue May 3, 2020
Adds support for windows via winscard. This CL leverages the
winscard api via calling into the dll. For more information
see https://docs.microsoft.com/en-us/windows/win32/api/winscard/.

Fixes #56

Signed-off-by: Tobias Kohlbau <tobias@kohlbau.de>
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 a pull request may close this issue.

2 participants