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

Add p11ez package. #56

Merged
merged 6 commits into from
Apr 23, 2018
Merged

Add p11ez package. #56

merged 6 commits into from
Apr 23, 2018

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Oct 14, 2017

This package wraps the main package in an easier and more idiomatic layer,
breaking out Go types that match some of the various concepts in PKCS#11.

I'm definitely open to alternate package names, though I like that this one
has "ez" (like easy) in the name and could alternately be pronounced
"pleez." :-)

One additional piece of work I might want to do is replace all of the structs
with interfaces. This would make it easier to mock them out, and also easier
to provide cutouts for a build tag like in #47.

This package wraps the main package in an easier and more idiomatic layer,
breaking out Go types that match some of the various concepts in PKCS#11.
@miekg
Copy link
Owner

miekg commented Oct 14, 2017

p11ez is p11easy? Why not shorten it to just p11?

Copy link
Owner

@miekg miekg left a comment

Choose a reason for hiding this comment

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

Code looks good. Mostly comments on naming - I think we should push this as far as possible to the Go standard of naming.

p11ez/module.go Outdated
}

// GetInfo returns general information about the module.
func (m Module) GetInfo() (pkcs11.Info, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

'Get' is not really idiomatic, just Info() and (if there is such a thing) SetInfo()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, will do this tomorrow.

p11ez/module.go Outdated
}

// Module represents a PKCS#11 module, and can be used to create Sessions.
type Module struct {
Copy link
Owner

Choose a reason for hiding this comment

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

Should we nanme this Context? Or would that be too confusing with context.Context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be too confusing with context.Context.

Copy link
Owner

Choose a reason for hiding this comment

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

Ack

Copy link
Owner

@miekg miekg left a comment

Choose a reason for hiding this comment

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

Yep looks good. We should do a
sed s/p11ex/p11

think just p11 is both simple, descriptive and short

@miekg
Copy link
Owner

miekg commented Nov 15, 2017

@jsha I really think we should just call this is p11 instead of p11ez. What do you think?

@jsha
Copy link
Contributor Author

jsha commented Nov 16, 2017 via email

@miekg
Copy link
Owner

miekg commented Feb 8, 2018

@jsha still working on this?

@jsha
Copy link
Contributor Author

jsha commented Feb 8, 2018 via email

Copy link

@comio comio left a comment

Choose a reason for hiding this comment

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

Any news? on this PR?

@jsha
Copy link
Contributor Author

jsha commented Apr 12, 2018

Sorry for the delay, all! I went ahead and pushed up what I had sitting in my local repo; I'll take another skim over things today and see where they stand.

@miekg
Copy link
Owner

miekg commented Apr 12, 2018 via email

@jsha
Copy link
Contributor Author

jsha commented Apr 12, 2018

Ok, I did a round of self review, mostly fixing up comments, and I think this is ready for another round of review.

One thing in particular I'm interested in feedback on: I'd like this package to be pretty flexible for most use cases, but not necessarily a full mirror of every function available in PKCS#11. For instance, I just removed GenerateKey on the theory that GenerateKeyPair is almost always what you want. Is there anything else that you think is super rare in practice, that we could remove to slim down the API (and possibly add back on demand)?

@miekg
Copy link
Owner

miekg commented Apr 23, 2018

I don't have an answer to your question, so I'll just merge :-)
We can always remove bits later and claim we don't guarantee backwards compat on the p11 part?

@miekg miekg merged commit e253b09 into miekg:master Apr 23, 2018
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