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 optional SSL for Gatekeeper #262

Merged
merged 1 commit into from
Jun 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion cmd/gatekeeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ import (
"github.com/manifoldco/torus-cli/gatekeeper"
)

var (
certFlag = newPlaceholder("cert, c", "CERT", "Certificate for SSL", "", "TORUS_GATEKEEPER_CERT", false)
keyFlag = newPlaceholder("key, k", "KEY", "Certificate key for SSL", "", "TORUS_GATEKEEPER_CERT_KEY", false)
)

func init() {
gatekeeper := cli.Command{
Name: "gatekeeper",
Expand All @@ -26,6 +31,12 @@ func init() {
Action: chain(ensureDaemon, ensureSession, loadDirPrefs,
loadPrefDefaults, startGatekeeperCmd,
),
Flags: []cli.Flag{
orgFlag("Use this organization by default in Gatekeeper", false),
roleFlag("Use this role.", false),
certFlag,
keyFlag,
},
},
},
}
Expand All @@ -42,9 +53,10 @@ func startGatekeeperCmd(ctx *cli.Context) error {
return errs.NewErrorExitError("Failed to load config.", err)
}

gatekeeper, err := gatekeeper.New(ctx.String("org"), ctx.String("team"), cfg)
gatekeeper, err := gatekeeper.New(ctx.String("org"), ctx.String("role"), ctx.String("cert"), ctx.String("key"), cfg)
if err != nil {
log.Printf("Error starting a new Gatekeeper instance: %s", err)
return err
}

log.Printf("v%s of the Gatekeeper is now listeneing on %s", cfg.Version, gatekeeper.Addr())
Expand Down
6 changes: 6 additions & 0 deletions cmd/machines.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ func authProviderFlag(usage string, required bool) cli.Flag {
return newPlaceholder("auth, a", "AUTHPROVIDER", usage, "", "TORUS_AUTH_PROVIDER", required)
}

func caFlag(usage string, required bool) cli.Flag {
return newPlaceholder("ca", "CA_BUNDLE", usage, "", "TORUS_BOOTSTRAP_CA", required)
}

func init() {
machines := cli.Command{
Name: "machines",
Expand Down Expand Up @@ -143,6 +147,7 @@ func init() {
roleFlag("Role the machine will belong to", true),
machineFlag("Machine name to bootstrap", false),
orgFlag("Org the machine will belong to", false),
caFlag("CA Bundle to use for certificate verification. Uses system if none is provided", false),
},
Action: chain(checkRequiredFlags, bootstrapCmd),
},
Expand Down Expand Up @@ -437,6 +442,7 @@ func bootstrapCmd(ctx *cli.Context) error {
ctx.String("machine"),
ctx.String("org"),
ctx.String("role"),
ctx.String("ca"),
)
if err != nil {
return fmt.Errorf("bootstrap provision failed: %s", err)
Expand Down
7 changes: 5 additions & 2 deletions gatekeeper/bootstrap/aws/bootstrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@ const (
)

// Bootstrap bootstraps a the AWS instance into a role to a given Gatekeeper instance
func Bootstrap(url, name, org, role string) (*apitypes.BootstrapResponse, error) {
func Bootstrap(url, name, org, role, caFile string) (*apitypes.BootstrapResponse, error) {
var err error
client := client.NewClient(url)
client, err := client.NewClient(url, caFile)
if err != nil {
return nil, fmt.Errorf("cannot initialize bootstrap client: %s", err)
}

identity, err := metadata()
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions gatekeeper/bootstrap/bootstrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ const (
)

// Do will execute the bootstrap request for the given provider
func Do(provider Provider, url, name, org, role string) (*apitypes.BootstrapResponse, error) {
func Do(provider Provider, url, name, org, role, caFile string) (*apitypes.BootstrapResponse, error) {
switch provider {
case AWSPublic:
return aws.Bootstrap(url, name, org, role)
return aws.Bootstrap(url, name, org, role, caFile)

default:
return nil, fmt.Errorf("invalid provider: %s", provider)
Expand Down
37 changes: 34 additions & 3 deletions gatekeeper/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ package client

import (
"context"
"crypto/tls"
"crypto/x509"
"fmt"
"io/ioutil"
"log"
"net/http"
"strings"
"time"
Expand All @@ -29,20 +33,47 @@ type Client struct {
}

// NewClient returns a new client to a Gatekeeper host that can bootstrap this machine
func NewClient(host string) *Client {
func NewClient(host, caFile string) (*Client, error) {
if !strings.HasSuffix(host, "/") {
host += "/"
}

caPool, err := x509.SystemCertPool()
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we log the error? As in, "Could not load System Cert Pool, error: %s, creating custom certificate pool"?

Also, thinking about this more, I think this would be fatal if a user didn't specify the --ca flag?

Copy link
Member Author

@enmand enmand Jun 20, 2017

Choose a reason for hiding this comment

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

Also, thinking about this more, I think this would be fatal if a user didn't specify the --ca flag?

Not sure here. There are a few cases I see:

  1. No --key and --cert given on gatekeeper cmd will start the Gatekeeper without SSL. Maybe they should be required too (--ca would probably need them to be required to be consistent).
  2. --key and --cert specify a certificate that is signed by the system bundle (or an intermediate cert), then --ca wouldn't be required.

Thinking on what OpenShift does, I think (1) makes sense. In OpenShift, they generate a CA cert (we'd have the user generate one, maybe?), and then use that as a basis of the certificates in the system. That way, there's no reliance on having a certain CA be trusted or not in your system. That should force Gatekeeper over SSL in all cases (which is probably the right thing)

Copy link
Contributor

Choose a reason for hiding this comment

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

For testing and development purposes, it makes sense for the Gatekeeper to not require SSL. However, in all other cases SSL should be required.

So, perhaps, we just add a log to create noise (a WARN) about the fact that SSL is not enabled.

Regarding the error, in the case where --ca is not provided and we can't get the SystemCertPool, we'd end up swallowing the error. Instead, I think this should be fatal instead of silently swallowed with an empty cert pool. Perhaps I'm missing some context?

log.Printf("Could not load system certificate pool: %s. Creating custom pool", err)
caPool = x509.NewCertPool()
}

if caFile != "" {
caBytes, err := ioutil.ReadFile(caFile)
if err != nil {
return nil, fmt.Errorf("unable to read ca file: %s", err)
}
ok := caPool.AppendCertsFromPEM(caBytes)
if !ok {
return nil, fmt.Errorf("unable to parse and append ca file: %s", err)
}
}

tlsConfig := &tls.Config{
RootCAs: caPool,
}

transport := &http.Transport{
TLSClientConfig: tlsConfig,
}

return &Client{
rt: &clientRoundTripper{
DefaultRequestDoer: registry.DefaultRequestDoer{
Client: &http.Client{
Timeout: clientTimeout,
Timeout: clientTimeout,
Transport: transport,
},
Host: host,
},
},
}
}, nil
}

// Bootstrap bootstraps the machine with Gatekeeper
Expand Down
7 changes: 5 additions & 2 deletions gatekeeper/gatekeeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@ import (
)

// New returns a new Gatekeeper
func New(org, team string, cfg *config.Config) (g *http.Gatekeeper, err error) {
func New(org, team, certpath, keypath string, cfg *config.Config) (g *http.Gatekeeper, err error) {
api := api.NewClient(cfg)
http := http.NewGatekeeper(org, team, cfg, api)
http, err := http.NewGatekeeper(org, team, certpath, keypath, cfg, api)
if err != nil {
return nil, err
}

return http, nil
}
46 changes: 44 additions & 2 deletions gatekeeper/http/http.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package http

import (
"crypto/tls"
"fmt"
"io/ioutil"
"log"
"net/http"

Expand Down Expand Up @@ -28,11 +31,27 @@ type Gatekeeper struct {
}

// NewGatekeeper returns a new Gatekeeper.
func NewGatekeeper(org, team string, cfg *config.Config, api *api.Client) *Gatekeeper {
func NewGatekeeper(org, team, certpath, keypath string, cfg *config.Config, api *api.Client) (*Gatekeeper, error) {
server := &http.Server{
Addr: cfg.GatekeeperAddress,
}

keypair, err := tlsKeypair(certpath, keypath)
if err != nil {
log.Printf("Starting Gatekeeper without SSL: %s", err)
} else {
if err != nil {
return nil, err
}

tlsConfig := &tls.Config{
MinVersion: tls.VersionTLS12,
Certificates: []tls.Certificate{*keypair},
}

server.TLSConfig = tlsConfig
}

g := &Gatekeeper{
defaults: gatekeeperDefaults{
Org: org,
Expand All @@ -43,7 +62,7 @@ func NewGatekeeper(org, team string, cfg *config.Config, api *api.Client) *Gatek
api: api,
}

return g
return g, nil
}

// Listen listens on a TCP port for HTTP machine requests
Expand Down Expand Up @@ -82,3 +101,26 @@ func loggingHandler(next http.Handler) http.Handler {
log.Printf("%s %s", r.Method, p)
})
}

func tlsKeypair(certpath, keypath string) (*tls.Certificate, error) {
if certpath == "" {
return nil, fmt.Errorf("no certificate provided")
}

certBytes, err := ioutil.ReadFile(certpath)
if err != nil {
return nil, fmt.Errorf("unable to read certificate: %s", err)
}

if keypath == "" {
return nil, fmt.Errorf("no certificate key provided")
}

keyBytes, err := ioutil.ReadFile(keypath)
if err != nil {
return nil, fmt.Errorf("unable to read keyfile: %s", err)
}

cert, err := tls.X509KeyPair(certBytes, keyBytes)
return &cert, err
}