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

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

Closed
wants to merge 1 commit into from

Conversation

stv0g
Copy link
Contributor

@stv0g stv0g commented Oct 8, 2023

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.

@stv0g stv0g changed the title pcsc: Replace custom code with github/com/ebfe/scard pcsc: Replace custom code with github/com/ebfe/scard Oct 8, 2023
@ericchiang
Copy link
Collaborator

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

This uses the ebfe/scard package to replace custom code
for interfacing with the PIV token on various platforms.
@stv0g
Copy link
Contributor Author

stv0g commented Oct 8, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

stv0g commented Oct 8, 2023

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
Copy link
Contributor Author

stv0g commented Oct 15, 2023

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

@stv0g stv0g closed this Oct 15, 2023
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.

2 participants