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

adding Shared access to multiple processes #100

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

rajnikant12345
Copy link

@rajnikant12345 rajnikant12345 commented Oct 13, 2021

Related: #47

Copy link
Collaborator

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

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

This probably won't work. Even if you have a shared connection, as soon as you open a transaction you block the card until the transaction is closed:

piv-go/piv/piv.go

Lines 162 to 169 in 4b80d66

tx, err := h.Begin()
if err != nil {
return nil, fmt.Errorf("beginning smart card transaction: %w", err)
}
if err := ykSelectApplication(tx, aidPIV[:]); err != nil {
tx.Close()
return nil, fmt.Errorf("selecting piv applet: %w", err)
}

We probably need to replace accesses to the YubiKey.tx field with a function that either reuses the one transaction in exclusive mode, or creates then closes the connection after.

func (yk *YubiKey) withTx(fn func(tx * scTx) error) error {
   // ...
}

Then functions go from

// Serial returns the YubiKey's serial number.
func (yk *YubiKey) Serial() (uint32, error) {
	return ykSerial(yk.tx, yk.version)
}

To

// Serial returns the YubiKey's serial number.
func (yk *YubiKey) Serial() (uint32, error) {
	var serial uint32
	err := yk.withTx(func(tx *scTx) error {
		var err error
		serial, err = ykSerial(tx, yk.version)
		return err
	})
	return serial, err
}

piv/pcsc_unix.go Outdated Show resolved Hide resolved
piv/pcsc_windows.go Outdated Show resolved Hide resolved
piv/piv.go Outdated Show resolved Hide resolved
@ericchiang
Copy link
Collaborator

@rajnikant12345 would you like me to take over this one?

@rajnikant12345
Copy link
Author

@rajnikant12345 would you like me to take over this one?

Yes that would be great, because this issue is moved to November on my side.

Thanks a lot

@rajnikant12345
Copy link
Author

rajnikant12345 commented Nov 3, 2021

@ericchiang , sorry for late checkin. I was busy with other related stories.
I created a pull request.Please review.
Whenever you get time.
Thanks

@rajnikant12345
Copy link
Author

rajnikant12345 commented Nov 8, 2021

@ericchiang is this already checked in from your side?

Thanks
Rajni

piv/key.go Outdated Show resolved Hide resolved
piv/key.go Outdated Show resolved Hide resolved
piv/pcsc_darwin.go Outdated Show resolved Hide resolved
piv/pcsc_linux.go Outdated Show resolved Hide resolved
piv/pcsc_unix.go Outdated Show resolved Hide resolved
piv/piv.go Outdated Show resolved Hide resolved
piv/piv.go Outdated Show resolved Hide resolved
piv/piv.go Outdated Show resolved Hide resolved
piv/piv.go Outdated Show resolved Hide resolved
@ericchiang
Copy link
Collaborator

Thanks for the follow up! Sorry for the delay, but please ping if you need me to run the CI

@rajnikant12345
Copy link
Author

rajnikant12345 commented Nov 22, 2021

Thanks @ericchiang for reviewing
I was busy with my new born 👶🏻, so got a bit delayed.
I tried fixing your review comments, please have a look.
we can run CI as well.
Thanks

@rajnikant12345
Copy link
Author

@ericchiang anything needed from my side on this?

@ezekiel
Copy link

ezekiel commented Aug 17, 2022

I see there are now some conflicts on the branch - if conflicts were resolved, could this be tested again and potentially merged?

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.

3 participants