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

feat: use mtls between frontend and backend #37

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

RobertRosca
Copy link
Member

@RobertRosca RobertRosca commented Nov 1, 2024

MR sets up API server to only communicate via mTLS and configures the frontend to proxy requests through with the required certificates.

TODO:

  • add documentation on creating the certificates
  • update frontend code to not use mTLS if certs are not provided
  • update frontend/backend to only bind to non-localhost interfaces on maxwell if mTLS is provided

@CammilleCC CammilleCC force-pushed the fix/real-time-updates branch from ca7a09e to 57215c4 Compare November 5, 2024 12:37
Base automatically changed from fix/real-time-updates to main November 5, 2024 12:50
@RobertRosca RobertRosca force-pushed the feat/real-time-updates-mtls branch 5 times, most recently from 55532f5 to b3f3d98 Compare November 7, 2024 13:24
@CammilleCC CammilleCC force-pushed the feat/real-time-updates-mtls branch from b3f3d98 to 4018916 Compare November 14, 2024 16:22
Copy link
Collaborator

@CammilleCC CammilleCC left a comment

Choose a reason for hiding this comment

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

TIL about creating and using certs, thanks for the initiative 😊

A few comments, but looks so good so far. Tested running the servers with and without certificates.

frontend/vite.config.ts Outdated Show resolved Hide resolved
api/src/damnit_api/settings.py Show resolved Hide resolved
frontend/vite.config.ts Outdated Show resolved Hide resolved
api/compose.yml Outdated Show resolved Hide resolved
)
}

const baseURL = new URL(VITE_URL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably avoid hardcoding the URL with the host and the port, as it might be best for it to be dynamic and accounting in which machine it is run (host), and the supplied port that could also come from the CLI argument.

The VITE_BASE_URL from before is for custom base path such as localhost:port/<BASE_URL>, which follows the following notation on https://vite.dev/config/shared-options.html#base. It is stored as environment variable as the proxies should also follow e.g., localhost:port/<BASE_URL>/graphql. Maybe the BASE_URL name was a bit misleading, sorry about that!

key: fs.readFileSync(path.resolve(__dirname, VITE_MTLS_KEY)),
cert: fs.readFileSync(path.resolve(__dirname, VITE_MTLS_CERT)),
ca: fs.readFileSync(path.resolve(__dirname, VITE_MTLS_CA)),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've tested locally, I found that adding these two helps making things compatible:

Suggested change
}
secureProtocol: "TLSv1_2_method",
ciphers: [
"TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA",
"TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384",
].join(":"),
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants