"veyron/services/identity": Enable "Secure" cookies
Since our mechanism for seeking blessings from our Blessing provider
is based on OAuth2 tokens bound to cookies, it is important to
protect cookies for our Blessing provider as best as we can.
Currently, our cookies are "HttpOnly" but not "Secure", which means
that the browser may send them on clear http channels. As per
RFC 6265 (http://tools.ietf.org/html/rfc6265), it is recommended that
cookies set over an https channels have the "Secure" attribute (See
Section 8.3 and 8.6)
This CL sets the "Secure" attribute on all our cookies, and adds
a test for it.
Change-Id: Id990860c9e4b8221f8071dbe610ca523f2849d12
diff --git a/services/identity/identityd/main.go b/services/identity/identityd/main.go
index 93df6b3..5d4773f 100644
--- a/services/identity/identityd/main.go
+++ b/services/identity/identityd/main.go
@@ -35,7 +35,7 @@
var (
// Flags controlling the HTTP server
httpaddr = flag.String("httpaddr", "localhost:8125", "Address on which the HTTP server listens on.")
- tlsconfig = flag.String("tlsconfig", "", "Comma-separated list of TLS certificate and private key files. If empty, will not use HTTPS.")
+ tlsconfig = flag.String("tlsconfig", "", "Comma-separated list of TLS certificate and private key files. This must be provided.")
host = flag.String("host", defaultHost(), "Hostname the HTTP server listens on. This can be the name of the host running the webserver, but if running behind a NAT or load balancer, this should be the host name that clients will connect to. For example, if set to 'x.com', Veyron identities will have the IssuerName set to 'x.com' and clients can expect to find the public key of the signer at 'x.com/pubkey/'.")
// Flags controlling auditing of Blessing operations.
@@ -128,7 +128,7 @@
}
})
vlog.Infof("Running HTTP server at: %v", httpaddress())
- go runHTTPServer(*httpaddr)
+ go runHTTPSServer(*httpaddr)
<-signals.ShutdownOnSignals()
}
@@ -205,7 +205,6 @@
return server, published, nil
}
-func enableTLS() bool { return len(*tlsconfig) > 0 }
func enableRandomHandler() bool {
return len(*googleConfigWeb)+len(*googleConfigChrome)+len(*googleConfigAndroid) == 0
}
@@ -241,12 +240,9 @@
}
return clientID, clientSecret, true
}
-func runHTTPServer(addr string) {
- if !enableTLS() {
- if err := http.ListenAndServe(addr, nil); err != nil {
- vlog.Fatalf("http.ListenAndServe failed: %v", err)
- }
- return
+func runHTTPSServer(addr string) {
+ if len(*tlsconfig) == 0 {
+ vlog.Fatal("Please set the --tlsconfig flag")
}
paths := strings.Split(*tlsconfig, ",")
if len(paths) != 2 {
@@ -306,11 +302,7 @@
if err != nil {
vlog.Fatalf("Failed to parse %q: %v", *httpaddr, err)
}
- scheme := "http"
- if enableTLS() {
- scheme = "https"
- }
- return fmt.Sprintf("%s://%s:%v", scheme, *host, port)
+ return fmt.Sprintf("https://%s:%v", *host, port)
}
func dumpAuditLog() {
diff --git a/services/identity/util/csrf.go b/services/identity/util/csrf.go
index 9fbe392..e4a8625 100644
--- a/services/identity/util/csrf.go
+++ b/services/identity/util/csrf.go
@@ -116,6 +116,7 @@
Value: b64encode(b),
Expires: time.Now().Add(time.Hour * 24),
HttpOnly: true,
+ Secure: true,
Path: "/",
}, b
}
diff --git a/services/identity/util/csrf_test.go b/services/identity/util/csrf_test.go
index 14ac43c..a6b81d4 100644
--- a/services/identity/util/csrf_test.go
+++ b/services/identity/util/csrf_test.go
@@ -1,6 +1,7 @@
package util
import (
+ "fmt"
"net/http"
"net/http/httptest"
"strings"
@@ -23,7 +24,10 @@
if err != nil {
t.Errorf("NewToken failed: %v", err)
}
- cookie := cookieSet(w, cookieName)
+ cookie, err := cookieVal(w, cookieName)
+ if err != nil {
+ t.Error(err)
+ }
if len(cookie) == 0 {
t.Errorf("Cookie should have been set. Request: [%v], Response: [%v]", r, w)
}
@@ -37,7 +41,10 @@
if _, err = c.MaybeSetCookie(w, r, failCookieName); err != nil {
t.Error("failed to create cookie: ", err)
}
- cookie = cookieSet(w, failCookieName)
+ cookie, err = cookieVal(w, failCookieName)
+ if err != nil {
+ t.Error(err)
+ }
if len(cookie) == 0 {
t.Errorf("Cookie should have been set. Request: [%v], Response: [%v]", r, w)
}
@@ -59,7 +66,11 @@
if err != nil {
t.Errorf("NewToken failed: %v", err)
}
- if len(cookieSet(w, cookieName)) > 0 {
+ cookie, err := cookieVal(w, cookieName)
+ if err != nil {
+ t.Error(err)
+ }
+ if len(cookie) > 0 {
t.Errorf("Cookie should not be set when it is already present. Request: [%v], Response: [%v]", r, w)
}
if err := c.ValidateToken(tok, r, cookieName, nil); err != nil {
@@ -84,7 +95,11 @@
if err != nil {
t.Errorf("NewToken failed: %v", err)
}
- if len(cookieSet(w, cookieName)) > 0 {
+ cookie, err := cookieVal(w, cookieName)
+ if err != nil {
+ t.Error(err)
+ }
+ if len(cookie) > 0 {
t.Errorf("Cookie should not be set when it is already present. Request: [%v], Response: [%v]", r, w)
}
var got int
@@ -101,12 +116,30 @@
}
}
-func cookieSet(w *httptest.ResponseRecorder, cookieName string) string {
- cookies := strings.Split(w.Header().Get("Set-Cookie"), ";")
- for _, c := range cookies {
- if strings.HasPrefix(c, cookieName) {
- return strings.TrimPrefix(c, cookieName+"=")
+func cookieVal(w *httptest.ResponseRecorder, cookieName string) (string, error) {
+ cookie := w.Header().Get("Set-Cookie")
+ if len(cookie) == 0 {
+ return "", nil
+ }
+ var (
+ val string
+ httpOnly, secure bool
+ )
+ for _, part := range strings.Split(cookie, "; ") {
+ switch {
+ case strings.HasPrefix(part, cookieName):
+ val = strings.TrimPrefix(part, cookieName+"=")
+ case part == "HttpOnly":
+ httpOnly = true
+ case part == "Secure":
+ secure = true
}
}
- return ""
+ if !httpOnly {
+ return "", fmt.Errorf("cookie for name %v is not HttpOnly", cookieName)
+ }
+ if !secure {
+ return "", fmt.Errorf("cookie for name %v is not Secure", cookieName)
+ }
+ return val, nil
}
