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

sha256_password auth support #808

Merged
merged 7 commits into from
Jun 1, 2018
Merged

sha256_password auth support #808

merged 7 commits into from
Jun 1, 2018

Conversation

julienschmidt
Copy link
Member

@julienschmidt julienschmidt commented May 25, 2018

Description

Fixes #625

It also adds the ability for caching_sha2_password authentication to use a local copy of the servers pub key.

This PR depends on #807, which has to be merged first (This PR will then be rebased and the changeset will be a lot smaller)

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@julienschmidt julienschmidt added this to the v1.4.0 milestone May 25, 2018
@julienschmidt julienschmidt mentioned this pull request May 26, 2018
5 tasks
@julienschmidt julienschmidt changed the base branch from master to auth_refactoring May 27, 2018 11:04
@julienschmidt julienschmidt mentioned this pull request May 27, 2018
2 tasks
@julienschmidt julienschmidt changed the base branch from auth_refactoring to master May 29, 2018 17:32
@julienschmidt julienschmidt changed the title [WIP] sha256_password auth support sha256_password auth support May 29, 2018
@julienschmidt julienschmidt requested a review from methane May 29, 2018 17:35
@methane
Copy link
Member

methane commented May 31, 2018

I don't like registry.
Now we have Connector interface. How about adding ServerPubKey only to Config object?

@julienschmidt
Copy link
Member Author

It's consistent with the existing code and we still support Go versions that do not support the Connector interface.

Even with the Connector interface it is still useful to be able to serialize a config to a DSN string. That is only possible if the keys etc are addressable by an identifier.

@methane
Copy link
Member

methane commented May 31, 2018

OK. But I don't want to use DSN and registry anymore.
I want Config.ServerPubKey *rsa.PublicKey.

Config.ServerPubKeyName string can be added separately for Config.FormatDSN().

@julienschmidt
Copy link
Member Author

That would require more changes to properly handle configurations that cannot be formatted as a DSN.
If you insist, I'm open to changing the exported struct attribute name to Config.ServerPubKeyName but adding an exported *rsa.PublicKey cannot happen has part of this pull request.

@julienschmidt
Copy link
Member Author

Using a config to format a DSN is by the way currently its only usage. I don't plan to add support for the Connector interface before the 1.4.0 release. This will require a few more thoughts and changes as well as proper testing for which we don't have time if the 1.4.0 release should happen soon.

Currently a config would thus be unusable as soon as someone adds a *rsa.PublicKey directly.

@methane
Copy link
Member

methane commented May 31, 2018

Then, how about ServerPublicKeyPath?
It's consistent with mysql --server-public-key-path commandline option.

@julienschmidt
Copy link
Member Author

It isn't a path but an identifier (name) for a registered key.
Unlike the mysql command line tool we never read any (file-)path.

Copy link
Member

@methane methane left a comment

Choose a reason for hiding this comment

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

OK. While I think registries are ugly workaround for DSN limitation, we can't deprecate it yet...

@julienschmidt julienschmidt merged commit d743639 into master Jun 1, 2018
@julienschmidt julienschmidt deleted the sha256_password branch June 1, 2018 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants