search

OpenSC / OpenSC.tokend

Tokend module for OS X with support for all cards supported by OpenSC
Other
36 stars 20 forks source link

Merge several enhancements (ECC, SHA-2, naming the token by CN) #28

Open mouse07410 opened 8 years ago

mouse07410 commented 8 years ago

This PR adds multiple enhancements and bug fixes, such as:

This PR tracks the current OpenSC master with the changes made in June and July 2016 (0.16.0 release, and drivers enhancements)

frankmorgner commented 8 years ago

Could you please rebase the PR to master and squash this into three reviewable commits one for ECC, one for SHA2, one for the CN? I refuse to look at 65 individual commits.

mouse07410 commented 8 years ago

Why don't you do the token labelling "correctly" inside the OpenSC "driver" part (in a similar way that is done in pkcs15-esteid.c) ?

Because of (a) insufficient knowledge on my part, (b) convenience (didn't have to mess with a whole another repo and potentially assume maintenance of yet another fork of another repo), (c) preference to rather depend on fewer things. My goal was to make it work with what I could control. I think I accomplished it.

Could you please rebase the PR to master and squash this into three reviewable commits one for ECC, one for SHA2, one for the CN?

I think lot of those 65 are yours, accumulated between November 2015 and last June. Given my rather weak Git skills, it's tempting to leave it as is. After all, the fork this PR is from works OK (unlike the repo the PR is for). I consider it a finished piece of work - maybe not the most beautiful one, but working properly for me and others who use it. And as long as there is one fork that does the job, I'm happy.

I refuse to look at 65 individual commits.

You reviewed many (if not most) of those commits already back when they were submitted. They haven't changed since. If memory serves, you weren't very happy with them then.

martinpaljak commented 8 years ago

I don't know the full result, but I'd be willing to go though the tokend related bits and pieces to get to a "meaningful" PR.

But I can say for starters that fixing the token name should be shifted to the PIV parts in OpenSC. I won't be touching the PIV part.

mouse07410 commented 8 years ago

I don't know the full result, but I'd be willing to go though the tokend related bits and pieces to get to a "meaningful" PR.

Excellent, thank you!

But I can say for starters that fixing the token name should be shifted to the PIV parts in OpenSC. I won't be touching the PIV part.

If you will add the token name capability to the PIV part of OpenSC, it is fine with me. Once it is there, I might remove it from my fork. But until that (token name in PIV/OpenSC) is fully functional and tested, I am not going to touch it in tokend.

I offered you working code (that does what the main trunk does not). Feel free to do with it what you see fit.

frankmorgner commented 7 years ago

@martinpaljak did you have any time for compiling the PR?