Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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
100 changes: 96 additions & 4 deletions cmd/vicadmin/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ import (
"compress/gzip"
"crypto/tls"
"crypto/x509"
"fmt"
"html/template"
"io"
"net"
"net/http"
"net/url"
Expand Down Expand Up @@ -75,6 +77,80 @@ const (
genericErrorMessage = "Internal Server Error; see /var/log/vic/vicadmin.log for details" // for http errors that shouldn't be displayed in the browser to the user
)

//wrapper struct around net.Conn for our custom funcs
type Conn struct {
net.Conn
b byte
err error
oneTimeSwitch bool
}

//Returns number of bytes read
func (c *Conn) Read(b []byte) (int, error) {
//oneTimeSwitch bool of first conn starts off as true so we do the TLS check once
if c.oneTimeSwitch {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about c.UncertainTLS for this flag (sorry to have you keep renaming it)? Then the code has some semantic meaning: "if c.UncertainTLS then determine whether TLS is present on this connection or not"

Hopefully you agree that makes the flag's purpose clearer?

c.oneTimeSwitch = false
b[0] = c.b
Copy link
Copy Markdown
Contributor

@gigawhitlocks gigawhitlocks May 17, 2018

Choose a reason for hiding this comment

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

just for the sake of sanity, before this access we probably want to check the length of b and return an error if it's 0 (maybe before we reset c.UncertainTLS actually)

// if there's more bytes to read
if len(b) > 1 && c.err == nil {
//recurse on next byte
n, e := c.Conn.Read(b[1:])
//close connection if error during reading
if e != nil {
c.Conn.Close()
}
//return total num of bytes read (+ current) and pass error e
return n + 1, e
}
//only one byte read
return 1, c.err
}
//using the default Conn read
return c.Conn.Read(b)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

// TLSRedirectLister ...

//wrapper struct for our custom funcs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's make this comment a bit more descriptive. What does ReqListener actually allow us to do?

type ReqListener struct {
Copy link
Copy Markdown
Contributor

@gigawhitlocks gigawhitlocks May 17, 2018

Choose a reason for hiding this comment

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

TLSRedirectListener ? Maybe something like that? Nobody's going to remember what a ReqListener is when they come back to this code in a couple years.

net.Listener
addr string
config *tls.Config
}

//override default listener Accept function and add TLS check
func (l *ReqListener) Accept() (net.Conn, error) {
//call default net.listener.Accept first
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor style nit: your comments lack a space after the // before the actual comment starts

c, err := l.Listener.Accept()
if err != nil {
return nil, err
}
//create slice of size 1 for storing the first byte
b := make([]byte, 1)
//regular conn read, b will be updated with byte val
_, err = c.Read(b)
if err != nil {
c.Close()
if err != io.EOF {
return nil, err
}
}
//wrap Conn in our custom struct
con := &Conn{
Conn: c,
b: b[0],
err: err,
oneTimeSwitch: true,
}
//the first byte is the hex byte 0x16 = 22
//which means that this is a TLS “handshake” record
if b[0] == 22 {
//HTTPS creates Conn from our custom con/tls config
return tls.Server(con, l.config), nil
}

//regular HTTP connection, return con
return con, nil
}

func (s *server) listen() error {
defer trace.End(trace.Begin(""))

Expand All @@ -95,7 +171,6 @@ func (s *server) listen() error {
}
if err != nil {
log.Errorf("Could not load certificate from config - running without TLS: %s", err)

s.l, err = net.Listen("tcp", s.addr)
return err
}
Expand Down Expand Up @@ -145,7 +220,7 @@ func (s *server) listen() error {
return err
}

s.l = tls.NewListener(innerListener, tlsconfig)
s.l = &ReqListener{Listener: innerListener, config: tlsconfig}
return nil
}

Expand All @@ -158,16 +233,33 @@ func (s *server) AuthenticatedHandle(link string, h http.Handler) {
s.Authenticated(link, h.ServeHTTP)
}

//Redirects HTTP to HTTPS
func HTTPSRedirectHandle(h http.Handler) http.Handler {
redirectToHTTPS := func(w http.ResponseWriter, r *http.Request) {
//If TLS is nil, request is not HTTPS, so we must redirect
if r.TLS == nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

delete this newline :P

http.Redirect(w, r, fmt.Sprintf("https://%s%s", r.Host, r.RequestURI), http.StatusMovedPermanently)
return
}
h.ServeHTTP(w, r)
return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks like a no-op return that can be deleted


}
return http.HandlerFunc(redirectToHTTPS)
}

func (s *server) Handle(link string, h http.Handler) {
log.Debugf("%s --- %s", time.Now().String(), link)
s.mux.Handle(link, gorillacontext.ClearHandler(h))
s.mux.Handle(link, gorillacontext.ClearHandler(HTTPSRedirectHandle(h)))
}

// Enforces authentication on route `link` and runs `handler` on successful auth
func (s *server) Authenticated(link string, handler func(http.ResponseWriter, *http.Request)) {
defer trace.End(trace.Begin(""))

authHandler := func(w http.ResponseWriter, r *http.Request) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove whitespace

// #nosec: Errors unhandled because it is okay if the cookie doesn't exist.
websession, _ := s.uss.cookies.Get(r, sessionCookieKey)

Expand Down Expand Up @@ -211,6 +303,7 @@ func (s *server) Authenticated(link string, handler func(http.ResponseWriter, *h
}

// user was authenticated via cert

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove whitespace

handler(w, r)
return
}
Expand Down Expand Up @@ -259,7 +352,6 @@ func (s *server) Authenticated(link string, handler func(http.ResponseWriter, *h
s.logoutHandler(w, r)
return
}

// if the date & remote IP on the cookie were valid, then the user is authenticated
log.Infof("User with a valid auth cookie at %s is authenticated.", connectingAddr[0])
handler(w, r)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ Get Login Page
${rc} ${output}= Run And Return Rc And Output curl -sk %{VIC-ADMIN}/authentication
Should contain ${output} <title>VCH Admin</title>

Check that HTTP Request Redirects to HTTPS
${rc} ${output}= Run And Return Rc And Output curl -L -sk -vvv http://%{VCH-IP}:2378
Should Contain ${output} SSL connection using TLS
Should Contain ${output} 301 Moved Permanently
Should not contain ${output} Empty reply from server

While Logged Out Fail To Display HTML
${rc} ${output}= Run And Return Rc And Output curl -sk %{VIC-ADMIN}
Should not contain ${output} <title>VIC: %{VCH-NAME}</title>
Expand Down