go-piv / piv-go

Keys and certificates for YubiKeys, written in Go
Apache License 2.0
366 stars 65 forks source link

pcsc: Replace custom code with `github/com/ebfe/scard` #131

Closed stv0g closed 1 year ago

stv0g commented 1 year ago

This uses the ebfe/scard package to replace custom code for interfacing with the PIV token on various platforms.

This work improves code reuse and interoperability when other code beside's go-piv needs to communicate with the smart card.

ericchiang commented 1 year ago

Hey @stv0g I don't plan on taking on external dependencies to this package. If there are any compatibility issues or bugs with piv-go, happy to discuss that though

stv0g commented 1 year ago

Hi @ericchiang,

Okay, I already suspected this :D What do you think about moving the PCSC-related code into its own module github.com/go-piv/piv-go/pcsc and using a simple interface for the token communication?

It could look like this:

type APDU struct {
    Instruction byte
    Param1      byte
    Param2      byte
    Data        []byte
}

type Card interface {
    Begin() (Transaction, error)
    Close() error
}

type Transaction interface {
    Transmit(d APDU) ([]byte, error)
    Close() error
}

This would also open the door to #85.

It would help the separation of concerns if the PIV related business logic is contained in github.com/go-piv/piv-go/piv while moving the smart card communication somewhere else, possibly also outside of the module.

Little background why I need this:

My code is already using github.com/ebfe/scard to communicate with other applets of the Yubikey (OATH, OpenPGP). Hence, I would like to avoid having two implementations of the winscard / pcsclite bindings.

ericchiang commented 1 year ago

My intuition is that for a change like that may want to fork this package and do these kind of modifications in a fork? I'm not sure that I want to commit to a pcsc interface.

And again, if you have any concrete reason for wanting to switch the pcsc package, I'm happy to discuss that and make changes here!

stv0g commented 1 year ago

Hi @ericchiang,

My intuition is that for a change like that may want to fork this package and do these kind of modifications in a fork?

Yes, I might start to maintain a fork using github.com/ebfe/scard here: https://github.com/cunicu/go-piv.

Nevertheless, I've started to work on an piv.Card/piv.Context interface just to see how it would turn out and whether we could avoid a fork.

See here: https://github.com/cunicu/go-piv/blob/scard-interface/piv/card.go

I'm not sure that I want to commit to a pcsc interface.

I think we can manage to introduce it without even breaking the current API of go-piv. The pcsc package in my branch above could be even internal internal/pcsc.

And again, if you have any concrete reason for wanting to switch the pcsc package, I'm happy to discuss that and make changes here!

I am aiming at maintaining only a single PC/SC context in my application and handle reader enumeration and card life cycle in my own code rather than go-piv. I know these might not be strong arguments but I am just uncomfortable with the idea of having two different winscard / libpcsclite bindings side-by-side in my code.

stv0g commented 1 year ago

Thanks for the consideration again. I will closed this issue as I will progress with a fork here: https://github.com/cunicu/go-piv