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

Move juju/cert pkg into utils. #250

Merged
merged 2 commits into from Nov 10, 2016
Merged

Move juju/cert pkg into utils. #250

merged 2 commits into from Nov 10, 2016

Conversation

hoenirvili
Copy link
Contributor

@hoenirvili hoenirvili commented Nov 10, 2016

As discussed in here #249
@axw @dooferlad

Signed-off-by: Salvatore Giulitti <sgiulitti@cloudbasesolutions.com>
Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

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

Just a few things to make this a bit more generic.


// NewCA generates a CA certificate/key pair suitable for signing server
// keys for an environment with the given name.
func NewCA(envName, UUID string, expiry time.Time) (certPEM, keyPEM string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is meant to be generic, I think we should just pass in the common name and use it directly in the CommonName field. juju/cert can then have its own NewCA function which calls the one in this package with fmt.Sprintf(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed !


// Verify verifies that the given server certificate is valid with
// respect to the given CA certificate at the given time.
func Verify(srvCertPEM, caCertPEM string, when time.Time) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

unless this is needed by winrm, I think we should leave this one in juju/cert. It has some juju-specifics, because of the "anyServer".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not needed for winrm, winrm itself has built it function to verify the cert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed !

}

// newLeaf generates a certificate/key pair suitable for use by a leaf node.
func newLeaf(caCertPEM, caKeyPEM string, expiry time.Time, hostnames []string, extKeyUsage []x509.ExtKeyUsage) (certPEM, keyPEM string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

as above, I think this should take CommonName as an arg, and juju/cert can have its own version that passes in "*"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed !

return n, nil
}

// NewDefaultServer generates a certificate/key pair suitable for use by a server, with an
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just keep these two functions (NewDefaultServer and NewServer) in juju/cert, and expose newLeaf (NewLeaf)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure ! Sounds reasonable.

Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

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

LGTM with a slight change. Thanks!

Signed-off-by: Salvatore Giulitti <sgiulitti@cloudbasesolutions.com>
@hoenirvili
Copy link
Contributor Author

$$merge$$

@jujubot
Copy link
Contributor

jujubot commented Nov 10, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-utils

@jujubot jujubot merged commit a84e60d into juju:master Nov 10, 2016
@hoenirvili hoenirvili deleted the move-cert-pkg branch November 10, 2016 13:56
jujubot added a commit to juju/juju that referenced this pull request Nov 11, 2016
Make all juju/cert paths point now to utils/cert

This patch depends on this patch to be merged juju/utils#250

This patch contains:
  -  moves  juju/cert into utils/ pkg.
  - update code path for all files.
  - update dependencies.tsv
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.

None yet

4 participants