Skip to content

Conversation

@jbrockopp
Copy link
Contributor

@jbrockopp jbrockopp commented May 22, 2022

This change enforces the API to utilize HTTP Strict-Transport-Security (HSTS):

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security

To accomplish this, we set a header to inform browsers that the site should only be accessed using HTTPS:

c.Header("Strict-Transport-Security", "max-age=63072000; includeSubDomains; preload")

NOTE: This is the same value we set for the go-vela/ui:

    add_header Strict-Transport-Security "max-age=63072000; includeSubDomains; preload" always;

This was flagged, as a low finding, by Cargill's security team during a penetration test of the system.

Their argument is by not enforcing secure connections, via HSTS, enables someone to exploit the product.

If an adversary were positioned in the middle of the endpoint and the server, they could use that against the system.

@jbrockopp jbrockopp added the bug Indicates a bug label May 22, 2022
@jbrockopp jbrockopp self-assigned this May 22, 2022
@codecov
Copy link

codecov bot commented May 22, 2022

Codecov Report

Merging #644 (62a9bc0) into master (8db1ffd) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #644      +/-   ##
==========================================
- Coverage   55.37%   55.36%   -0.01%     
==========================================
  Files         195      195              
  Lines       15830    15827       -3     
==========================================
- Hits         8766     8763       -3     
  Misses       6695     6695              
  Partials      369      369              
Impacted Files Coverage Δ
router/middleware/header.go 56.36% <100.00%> (-2.26%) ⬇️

@jbrockopp jbrockopp marked this pull request as ready for review May 22, 2022 21:49
@jbrockopp jbrockopp requested a review from a team as a code owner May 22, 2022 21:49
Copy link
Contributor

@kneal kneal left a comment

Choose a reason for hiding this comment

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

LGTM 🐬

@wass3r wass3r merged commit 68ad2a1 into master May 25, 2022
@wass3r wass3r deleted the fix/api/hsts_header branch May 25, 2022 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Indicates a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants