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

Appliance shared TLS certificate #1272

Merged
merged 1 commit into from
Feb 9, 2018

Conversation

andrewtchin
Copy link
Contributor

@andrewtchin andrewtchin commented Dec 19, 2017

Fixes #881
Fixes #1007
Towards #1179

tested with
generated self signed DHCP
generated self signed static
custom cert DHCP
custom cert static

@andrewtchin andrewtchin requested a review from a user December 20, 2017 20:10
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Lgtm. Is it possible to use this cert for admiral and harbor too?

@andrewtchin
Copy link
Contributor Author

yes i will definitely be making sure everything on the appliance uses this. i'm waiting until your systemd refactor merges first :-)

@andrewtchin andrewtchin changed the title WIP Shared TLS certificates Appliance shared TLS certificate Jan 19, 2018
@andrewtchin
Copy link
Contributor Author

make sure this survives rebase #1381

@andrewtchin
Copy link
Contributor Author

screen shot 2018-02-02 at 3 42 22 pm

@andrewtchin
Copy link
Contributor Author

@morris-jason ready for another review

Copy link

@ghost ghost 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 some comments.

@@ -5,6 +5,8 @@ Documentation=https://github.com/vmware/vic-product
[Service]
Type=oneshot
ExecStart=/etc/vmware/vic-appliance-environment.sh
Requires=ovf-network.service
Copy link

Choose a reason for hiding this comment

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

probably want network-online.target here too.


func getTLSCertFingerprint() string {
certFile := "/storage/data/certs/server.crt"
if _, err := os.Stat(certFile); err == nil {
Copy link

Choose a reason for hiding this comment

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

Could this be simplified to:

func getTLSCertFingerprint() string {
    certFile := "/storage/data/certs/server.crt"
    certPEM, e := ioutil.ReadFile(certFile)
    if e != nil {
        log.Debugf("Read error: %s", e.Error())
        return certNotAvail
    }
    block, _ := pem.Decode([]byte(certPEM))
    if block == nil {
        log.Debugf("Parse error")
        return certNotAvail
    }
    cert, err := x509.ParseCertificate(block.Bytes)
    if err != nil {
        log.Debugf("Parse error: %s", err.Error())
        return certNotAvail
    }
return fmt.Sprintf("% X", sha1.Sum(cert.Raw))

if err != nil {
return fmt.Sprintf("Parse error: %s", err.Error())
}
return fmt.Sprintf("% X", sha1.Sum(cert.Raw))
Copy link

Choose a reason for hiding this comment

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

Should this be "%X" instead of "% X"? Does the extra space break this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

space adds a space after each octet for readability

- Shares the same TLS certificate for all services running on the appliance
- Places the certificate in a well known location
- Simplify manifest for files included in appliance
- Displays cert fingerprint to DCUI
- Automatically convert private key to correct format
- Update appliance-support.sh and SUPPORT.md
- Move hostname detection for cert generation to vic-appliance-environment
@andrewtchin andrewtchin merged commit 9423ab0 into vmware:master Feb 9, 2018
@andrewtchin andrewtchin deleted the 1179/landing-page branch February 9, 2018 18:50
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