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 wrappers to enable/disable Padding for encryption contexts #99

Merged
merged 1 commit into from
Aug 17, 2018
Merged

add wrappers to enable/disable Padding for encryption contexts #99

merged 1 commit into from
Aug 17, 2018

Conversation

rameshrv
Copy link
Contributor

We needed to disable and use customized padding in our product. This code change enables us (and others who need it) to have custom padding.

@hasit
Copy link

hasit commented Aug 16, 2018

I need this as well. We need to turn off the default padding and do it our way because we need to comply with our clients.

@zeebo
Copy link
Member

zeebo commented Aug 16, 2018

Thanks for this contribution. I have some changes I'd like to request in order to get it merged:

  • Can you change the method to be SetPadding(bool) rather than two methods?
  • We can't add methods to an exported interface due to back-compat concerns. Your local code can still type assert to get access to the new method.

Let me know if you're willing to make these changes. Be sure to add yourself to the AUTHORS file as well. Thanks!

@rameshrv
Copy link
Contributor Author

@zeebo , thanks ! I will do the changes as you suggested and update the PR.

@hasit
Copy link

hasit commented Aug 16, 2018

@zeebo, can you please expand on what you meant here?

We can't add methods to an exported interface due to back-compat concerns. Your local code can still type assert to get access to the new method.

@zeebo
Copy link
Member

zeebo commented Aug 16, 2018

A package depending on this package may have a local implementation of the interface on some type and adding methods to it would cause that package to no longer compile because the local implementation would not have that method.

I try to be really careful about back compat. That's why https://godoc.org/github.com/spacemonkeygo/openssl#LoadPrivateKeyFromPEMWidthPassword still exists. :)

@rameshrv
Copy link
Contributor Author

@zeebo just pushed the changes. However, I am still unclear of this: SetPadding will still be not accessible outside (as of my current changes) but you say we could type assert and access it from applications ? Can you please elaborate on that ? I have tried to type assert it as EncryptionCipherCtx or CipherCtx, it never works. I will need to put it in one of the external structs/interfaces to access it right ?

@zeebo
Copy link
Member

zeebo commented Aug 16, 2018

Here's what I mean: https://play.golang.org/p/bqyEPtjsWB3

@rameshrv
Copy link
Contributor Author

@zeebo , thank you for the reference ! It works like a charm..I have never used an exported method in that way !

I have now changed the AUTHORS file, as well as squashed my previous commits into one single commit. Do you think the current changes are good enough to merge into the main line ?

@zeebo
Copy link
Member

zeebo commented Aug 17, 2018

Perfect! Thanks for the work.

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