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.

Sign up for GitHub

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

feat(ui): Enable CSP, HSTS, X-Frame-Options. Fixes #2760, #1376, #2761 #2971

Merged
merged 3 commits into from
May 13, 2020
Merged

feat(ui): Enable CSP, HSTS, X-Frame-Options. Fixes #2760, #1376, #2761 #2971

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd40d05
feat(ui) Enable CSP, HSTS, X-Frame-Options. Fixes #2760, #1376, #2761
alexec May 7, 2020
625a2f2
Merge branch 'master' into feat-csp
alexec May 8, 2020
c2470fa
feat-csp
alexec May 8, 2020
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
2 changes: 1 addition & 1 deletion server/apiserver/argoserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ func (as *argoServer) newHTTPServer(ctx context.Context, port int, artifactServe
mux.Handle("/api/", gwmux)
mux.HandleFunc("/artifacts/", artifactServer.GetArtifact)
mux.HandleFunc("/artifacts-by-uid/", artifactServer.GetArtifactByUID)
mux.HandleFunc("/", static.NewFilesServer(as.baseHRef).ServerFiles)
mux.HandleFunc("/", static.NewFilesServer(as.baseHRef, as.tlsConfig != nil).ServerFiles)
return &httpServer
}

Expand Down
11 changes: 9 additions & 2 deletions server/static/static.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ import (

type FilesServer struct {
baseHRef string
secure bool
}

func NewFilesServer(baseHRef string) *FilesServer {
return &FilesServer{baseHRef}
func NewFilesServer(baseHRef string, secure bool) *FilesServer {
return &FilesServer{baseHRef, secure}
}

func (s *FilesServer) ServerFiles(w http.ResponseWriter, r *http.Request) {
Expand All @@ -26,6 +27,12 @@ func (s *FilesServer) ServerFiles(w http.ResponseWriter, r *http.Request) {
w = &responseRewriter{ResponseWriter: w, old: []byte(`<base href="/">`), new: []byte(fmt.Sprintf(`<base href="%s">`, s.baseHRef))}
}

w.Header().Set("X-Frame-Options", "DENY")
w.Header().Set("Content-Security-Policy", "default-src 'self' 'unsafe-inline'")
if s.secure {
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, HSTS should be set from the final endpoint to the client (i.e. the LB or Ingress), because of several reasons. For one, once set, the browser will not access the URL on http endpoint anymore but will restrict to using https connection. Might lead to edge-case problems, i.e. Client -> (http) -> LB and/or Ingress -> (https) -> Argo)

But the more obvious reason might be, that the connection is Client -> (https) -> LB and/or Ingress -> (http) -> Argo). In this case, the HSTS header would not be set (because Argo server is serving plain HTTP), and then this might have no effect at all.

I think a better solution would be to document that users should set HSTS header on their Load Balancer or Ingress, if they want it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point - and you can front this with HTTP LB, in which case you would never want HSTS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I note your second case cannot apply - if we serve on HTTP - this flag won't be added - but I think we should make it optional - default on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, so new logic is this - --hsts is on by default - but only effected if --secure

w.Header().Set("Strict-Transport-Security", "max-age=31536000")
}

// in my IDE (IntelliJ) the next line is red for some reason - but this is fine
ServeHTTP(w, r)
}
Binary file added ui/src/app/assets/fonts/Heebo-Regular.ttf
View file
Open in desktop
Binary file not shown.
18 changes: 18 additions & 0 deletions ui/src/app/assets/styles/heebo.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/* hebrew */
@font-face {
font-family: 'Heebo';
font-style: normal;
font-weight: 400;
font-display: swap;
src: local('Heebo'), local('Heebo-Regular'), url(../fonts/Heebo-Regular.ttf) format('ttf');
unicode-range: U+0590-05FF, U+20AA, U+25CC, U+FB1D-FB4F;
}
/* latin */
@font-face {
font-family: 'Heebo';
font-style: normal;
font-weight: 400;
font-display: swap;
src: local('Heebo'), local('Heebo-Regular'), url(../fonts/Heebo-Regular.ttf) format('ttf');
unicode-range: U+0000-00FF, U+0131, U+0152-0153, U+02BB-02BC, U+02C6, U+02DA, U+02DC, U+2000-206F, U+2074, U+20AC, U+2122, U+2191, U+2193, U+2212, U+2215, U+FEFF, U+FFFD;
}
2 changes: 1 addition & 1 deletion ui/src/app/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<meta charset="UTF-8">
<title>Argo</title>
<base href="/">
<link href="https://fonts.googleapis.com/css?family=Heebo:300,400,500,700" rel="stylesheet">
<link href="assets/styles/heebo.css" rel="stylesheet">
<meta name="viewport" content="width=device-width, initial-scale=1">
<meta name="robots" content="noindex">
<link rel="icon" type="image/png" href="assets/favicon/favicon-32x32.png" sizes="32x32">
Expand Down
2 changes: 2 additions & 0 deletions ui/src/app/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ const config = {
from: 'node_modules/argo-ui/src/assets', to: 'assets'
}, {
from: 'node_modules/@fortawesome/fontawesome-free/webfonts', to: 'assets/fonts'
}, {
from: 'src/app/assets', to: 'assets'
}]),
],
devServer: {
Expand Down