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

Allow users to create TLS config from arbitrary sources of PEM data #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simonferquel
Copy link

Fix #52

This introduce an abstraction for accessing PEM Data (that can come from files, or from anything else, including byte slice in memory).

Fix docker#52

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
@thaJeztah
Copy link
Member

Wow, that's a lot of changes.

Just some quick thoughts;

Basically we want to replace tls.LoadX509KeyPair() (currently used, assumes a filename is passed), with tls.X509KeyPair() which just loads the data, correct?

So for the config() function-parameters, if this part of the code didn't expect it has to load a file, but checks if ..Certificates is already propagated;

tlsConfig := ServerDefault()
tlsConfig.ClientAuth = options.ClientAuth
tlsCert, err := tls.LoadX509KeyPair(options.CertFile, options.KeyFile)
if err != nil {
if os.IsNotExist(err) {
return nil, fmt.Errorf("Could not load X509 key pair (cert: %q, key: %q): %v", options.CertFile, options.KeyFile, err)
}
return nil, fmt.Errorf("Error reading X509 key pair (cert: %q, key: %q): %v. Make sure the key is not encrypted.", options.CertFile, options.KeyFile, err)
}
tlsConfig.Certificates = []tls.Certificate{tlsCert}

Would it then be possible to just load the data upfront?

Just really thinking out loud

@simonferquel
Copy link
Author

@thaJeztah

  • problem with config, is that the option struct itself assumes it references files. So we would need to modify everything with the assumption that everything has already been loaded.
  • ReaderCloser implementations can only be read once. I can't guarantee that an option object ould be used only once (and anyway, I am not completely sure that some files are not already read twice in some Client or Server calls).

I also wanted to keep the error messages as untouched as possible (the only purpose of the PEMSource interface to have a Name() method is to keep the same error messages everywhere it is possible)

@vdemeester
Copy link
Collaborator

I think @thaJeztah thinks more of something like the following diff : master...vdemeester:more-opts

This would allow to pass read/load certificates from memory (or really anywhere) without requiring any more changes in that repository for that matter.

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.

None yet

3 participants