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

refactor(server): load secrets from file instead of env var #972

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 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
9 changes: 5 additions & 4 deletions packages/server/knexfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const { packageRoot } = require('./bootstrap')
const fs = require('fs')
const path = require('path')
const { isTestEnv } = require('@/modules/shared/helpers/envHelper')
const { getPostgresUrl } = require('@/modules/shared/helpers/secretsHelper')

function walk(dir) {
let results = []
Expand Down Expand Up @@ -42,11 +43,11 @@ let connectionUri
if (env.POSTGRES_USER && env.POSTGRES_PASSWORD) {
connectionUri = `postgres://${encodeURIComponent(
env.POSTGRES_USER
)}:${encodeURIComponent(env.POSTGRES_PASSWORD)}@${
env.POSTGRES_URL
}/${encodeURIComponent(env.POSTGRES_DB)}`
)}:${encodeURIComponent(
env.POSTGRES_PASSWORD
)}@${getPostgresUrl()}/${encodeURIComponent(env.POSTGRES_DB)}`
} else {
connectionUri = env.POSTGRES_URL
connectionUri = getPostgresUrl()
}

// NOTE: fixes time pagination, breaks graphql DateTime parsing :/
Expand Down
5 changes: 3 additions & 2 deletions packages/server/modules/auth/strategies.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict'

const redis = require('redis')
const { getRedisUrl, getSessionSecret } = require('../shared/helpers/secretsHelper')
const ExpressSession = require('express-session')
const RedisStore = require('connect-redis')(ExpressSession)
const passport = require('passport')
Expand All @@ -20,8 +21,8 @@ module.exports = async (app) => {
app.use(passport.initialize())

const session = ExpressSession({
store: new RedisStore({ client: redis.createClient(process.env.REDIS_URL) }),
secret: process.env.SESSION_SECRET,
store: new RedisStore({ client: redis.createClient(getRedisUrl()) }),
secret: getSessionSecret(),
saveUninitialized: false,
resave: false,
cookie: { maxAge: 1000 * 60 * 3 } // 3 minutes
Expand Down
3 changes: 2 additions & 1 deletion packages/server/modules/auth/strategies/azure-ad.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const {
resolveAuthRedirectPath
} = require('@/modules/serverinvites/services/inviteProcessingService')
const { passportAuthenticate } = require('@/modules/auth/services/passportService')
const { getAzureADClientSecret } = require('@/modules/shared/helpers/secretsHelper')

module.exports = async (app, session, sessionStorage, finalizeAuth) => {
const strategy = new OIDCStrategy(
Expand All @@ -27,7 +28,7 @@ module.exports = async (app, session, sessionStorage, finalizeAuth) => {
process.env.CANONICAL_URL
).toString(),
allowHttpForRedirectUrl: true,
clientSecret: process.env.AZURE_AD_CLIENT_SECRET,
clientSecret: getAzureADClientSecret(),
scope: ['profile', 'email', 'openid'],
loggingLevel: process.env.NODE_ENV === 'development' ? 'info' : 'error',
passReqToCallback: true
Expand Down
3 changes: 2 additions & 1 deletion packages/server/modules/auth/strategies/github.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const {
resolveAuthRedirectPath
} = require('@/modules/serverinvites/services/inviteProcessingService')
const { passportAuthenticate } = require('@/modules/auth/services/passportService')
const { getGitHubClientSecret } = require('@/modules/shared/helpers/secretsHelper')

module.exports = async (app, session, sessionStorage, finalizeAuth) => {
const strategy = {
Expand All @@ -27,7 +28,7 @@ module.exports = async (app, session, sessionStorage, finalizeAuth) => {
const myStrategy = new GithubStrategy(
{
clientID: process.env.GITHUB_CLIENT_ID,
clientSecret: process.env.GITHUB_CLIENT_SECRET,
clientSecret: getGitHubClientSecret(),
callbackURL: new URL(strategy.callbackUrl, process.env.CANONICAL_URL).toString(),
scope: ['profile', 'user:email'],
passReqToCallback: true
Expand Down
3 changes: 2 additions & 1 deletion packages/server/modules/auth/strategies/google.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const {
resolveAuthRedirectPath
} = require('@/modules/serverinvites/services/inviteProcessingService')
const { passportAuthenticate } = require('@/modules/auth/services/passportService')
const { getGoogleClientSecret } = require('@/modules/shared/helpers/secretsHelper')

module.exports = async (app, session, sessionStorage, finalizeAuth) => {
const strategy = {
Expand All @@ -25,7 +26,7 @@ module.exports = async (app, session, sessionStorage, finalizeAuth) => {
const myStrategy = new GoogleStrategy(
{
clientID: process.env.GOOGLE_CLIENT_ID,
clientSecret: process.env.GOOGLE_CLIENT_SECRET,
clientSecret: getGoogleClientSecret(),
callbackURL: strategy.callbackUrl,
scope: ['profile', 'email'],
passReqToCallback: true
Expand Down
5 changes: 2 additions & 3 deletions packages/server/modules/blobstorage/objectStorage.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const { NotFoundError } = require('@/modules/shared/errors')
const { getS3SecretKey } = require('@/modules/shared/helpers/secretsHelper')
const {
S3Client,
GetObjectCommand,
Expand All @@ -15,13 +16,11 @@ const getS3Config = () => {
if (!s3Config) {
if (!process.env.S3_ACCESS_KEY)
throw new Error('Config value S3_ACCESS_KEY is missing')
if (!process.env.S3_SECRET_KEY)
throw new Error('Config value S3_SECRET_KEY is missing')
if (!process.env.S3_ENDPOINT) throw new Error('Config value S3_ENDPOINT is missing')
s3Config = {
credentials: {
accessKeyId: process.env.S3_ACCESS_KEY,
secretAccessKey: process.env.S3_SECRET_KEY
secretAccessKey: getS3SecretKey()
},
endpoint: process.env.S3_ENDPOINT,
forcePathStyle: true,
Expand Down
3 changes: 2 additions & 1 deletion packages/server/modules/emails/utils/transporter.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const debug = require('debug')('speckle')

const { getEmailPassword } = require('@/modules/shared/helpers/secretsHelper')
const nodemailer = require('nodemailer')
const modulesDebug = debug.extend('modules')
const errorDebug = debug.extend('errors')
Expand All @@ -20,7 +21,7 @@ const initSmtpTransporter = async () => {
secure: process.env.EMAIL_SECURE === 'true',
auth: {
user: process.env.EMAIL_USERNAME,
pass: process.env.EMAIL_PASSWORD
pass: getEmailPassword()
}
})
await smtpTransporter.verify()
Expand Down
6 changes: 6 additions & 0 deletions packages/server/modules/shared/errors/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ export class MisconfiguredEnvironmentError extends BaseError {
'An error occurred due to the server environment being misconfigured'
}

export class MisconfiguredSecretError extends BaseError {
static code = 'MISONFIGURED_SECRET_ERROR'
static defaultMessage =
'An error occurred due to the server secret file being misconfigured'
}

export class UninitializedResourceAccessError extends BaseError {
static code = 'UNINITIALIZED_RESOURCE_ACCESS_ERROR'
static defaultMessage = 'Attempted to use uninitialized resources'
Expand Down
2 changes: 1 addition & 1 deletion packages/server/modules/shared/helpers/bullHelper.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Redis from 'ioredis'
import Bull from 'bull'
import { getRedisUrl } from '@/modules/shared/helpers/envHelper'
import { getRedisUrl } from '@/modules/shared/helpers/secretsHelper'

export function buildBaseQueueOptions(): Bull.QueueOptions {
return {
Expand Down
8 changes: 0 additions & 8 deletions packages/server/modules/shared/helpers/envHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,6 @@ export function getFileSizeLimitMB() {
return parseInt(process.env.FILE_SIZE_LIMIT_MB || '100')
}

export function getRedisUrl() {
if (!process.env.REDIS_URL) {
throw new MisconfiguredEnvironmentError('REDIS_URL env var not configured')
}

return process.env.REDIS_URL
}

/**
* Get app base url / canonical url / origin
*/
Expand Down
75 changes: 75 additions & 0 deletions packages/server/modules/shared/helpers/secretsHelper.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import { MisconfiguredSecretError } from '@/modules/shared/errors'
import { existsSync, readFile } from 'fs'
import { join } from 'path'

let redisUrl: string
let postgresUrl: string
let sessionSecret: string
let emailPassword: string
let s3SecretKey: string
let googleClientSecret: string
let githubClientSecret: string
let azureADClientSecret: string
let apolloKey: string

export function getRedisUrl(): string {
if (!redisUrl) redisUrl = getSecret('redis_url')
return redisUrl
}

export function getPostgresUrl(): string {
if (!postgresUrl) postgresUrl = getSecret('postgres_url')
return postgresUrl
}

export function getSessionSecret(): string {
if (!sessionSecret) sessionSecret = getSecret('session_secret')
return sessionSecret
}

export function getEmailPassword(): string {
if (!emailPassword) emailPassword = getSecret('email_password')
return emailPassword
}

export function getS3SecretKey(): string {
if (!s3SecretKey) s3SecretKey = getSecret('s3_secret_key')
return s3SecretKey
}

export function getGoogleClientSecret(): string {
if (!googleClientSecret) googleClientSecret = getSecret('google_client_secret')
return googleClientSecret
}

export function getGitHubClientSecret(): string {
if (!githubClientSecret) githubClientSecret = getSecret('github_client_secret')
return githubClientSecret
}

export function getAzureADClientSecret(): string {
if (!azureADClientSecret) azureADClientSecret = getSecret('azure_ad_client_secret')
return azureADClientSecret
}

export function getApolloKey(): string {
if (!apolloKey) apolloKey = getSecret('apollo_key')
return apolloKey
}

function getSecret(secretName: string): string {
const secretPath = join('/etc/secrets', secretName)
Copy link
Contributor

@fabis94 fabis94 Aug 29, 2022

Choose a reason for hiding this comment

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

(How?) Will this work on local development environments? Also I'm wondering if this shouldn't be an infra only solution (e.g. some cloud feature that stores some env vars separately, but when they reach the app theyre just env vars), not something that changes how our app reads environment config values? Just considering that this seems like a security feature only relevant for the prod env

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for challenging this, it led to a better solution.
To summarise what we discussed for future reference; the file won't exist on a dev environment, so this will fallback to using environment variables. It is backwards-compatible with our current dev & test setups.
On production the files would exist, so we wouldn't need the secrets in environment variables (secrets in env vars are relatively insecure).

if (existsSync(secretPath)) {
readFile(secretPath, { encoding: 'utf-8' }, function (err, data) {
if (!err) return data
})
}

if (process.env[secretName.toUpperCase()]) {
return process.env[secretName.toUpperCase()] ?? ''
}

throw new MisconfiguredSecretError(
`Neither ${secretPath} or ${secretName.toUpperCase()} env var were configured`
)
}
5 changes: 3 additions & 2 deletions packages/server/modules/shared/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const { ForbiddenError, ApolloError } = require('apollo-server-express')
const { RedisPubSub } = require('graphql-redis-subscriptions')
const { buildRequestLoaders } = require('@/modules/core/loaders')
const { validateToken } = require(`@/modules/core/services/tokens`)
const { getRedisUrl } = require('@/modules/shared/helpers/secretsHelper')

const StreamPubsubEvents = Object.freeze({
UserStreamAdded: 'USER_STREAM_ADDED',
Expand All @@ -17,8 +18,8 @@ const StreamPubsubEvents = Object.freeze({
* GraphQL Subscription PubSub instance
*/
const pubsub = new RedisPubSub({
publisher: new Redis(process.env.REDIS_URL),
subscriber: new Redis(process.env.REDIS_URL)
publisher: new Redis(getRedisUrl()),
subscriber: new Redis(getRedisUrl())
})

/**
Expand Down
55 changes: 15 additions & 40 deletions utils/helm/speckle-server/templates/server/deployment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ spec:
- name: postgres-certificate
mountPath: /postgres-certificate
{{- end }}
- name: secrets
mountPath: "/etc/secrets/"
readOnly: true

# Allow for k8s to remove the pod from the service endpoints to stop receive traffic
lifecycle:
Expand Down Expand Up @@ -95,28 +98,16 @@ spec:
- name: DEBUG
value: "speckle:*"

- name: SESSION_SECRET
valueFrom:
secretKeyRef:
name: "{{ .Values.secretName }}"
key: session_secret
# SESSION_SECRET mounted as file

- name: FILE_SIZE_LIMIT_MB
value: {{ .Values.file_size_limit_mb | quote }}

# *** Redis ***
- name: REDIS_URL
valueFrom:
secretKeyRef:
name: {{ .Values.secretName }}
key: redis_url
# REDIS_URL mounted as file

# *** PostgreSQL Database ***
- name: POSTGRES_URL
valueFrom:
secretKeyRef:
name: {{ .Values.secretName }}
key: postgres_url
# POSTGRES_URL mounted as file
- name: POSTGRES_MAX_CONNECTIONS_SERVER
value: {{ .Values.db.maxConnectionsServer | quote }}

Expand All @@ -136,11 +127,7 @@ spec:
value: {{ .Values.s3.access_key }}
- name: S3_BUCKET
value: {{ .Values.s3.bucket }}
- name: S3_SECRET_KEY
valueFrom:
secretKeyRef:
name: {{ .Values.secretName }}
key: s3_secret_key
# S3_SECRET_KEY mounted as file
- name: S3_CREATE_BUCKET
value: "{{ .Values.s3.create_bucket }}"
- name: S3_REGION
Expand All @@ -165,11 +152,7 @@ spec:
value: "true"
- name: GOOGLE_CLIENT_ID
value: {{ .Values.server.auth.google.client_id }}
- name: GOOGLE_CLIENT_SECRET
valueFrom:
secretKeyRef:
name: {{ .Values.secretName }}
key: google_client_secret
# GOOGLE_CLIENT_SECRET mounted as file
{{- end }}

# Github Auth
Expand All @@ -178,11 +161,7 @@ spec:
value: "true"
- name: GITHUB_CLIENT_ID
value: {{ .Values.server.auth.github.client_id }}
- name: GITHUB_CLIENT_SECRET
valueFrom:
secretKeyRef:
name: {{ .Values.secretName }}
key: github_client_secret
# GITHUB_CLIENT_SECRET mounted as file
{{- end }}

# AzureAD Auth
Expand All @@ -197,11 +176,7 @@ spec:
value: {{ .Values.server.auth.azure_ad.issuer }}
- name: AZURE_AD_CLIENT_ID
value: {{ .Values.server.auth.azure_ad.client_id }}
- name: AZURE_AD_CLIENT_SECRET
valueFrom:
secretKeyRef:
name: {{ .Values.secretName }}
key: azure_ad_client_secret
# AZURE_AD_CLIENT_SECRET mounted as file
{{- end }}


Expand All @@ -216,11 +191,7 @@ spec:
value: "{{ .Values.server.email.port }}"
- name: EMAIL_USERNAME
value: "{{ .Values.server.email.username }}"
- name: EMAIL_PASSWORD
valueFrom:
secretKeyRef:
name: {{ .Values.secretName }}
key: email_password
# EMAIL_PASSWORD mounted as file
{{- end }}

# *** Tracking / Tracing ***
Expand Down Expand Up @@ -285,6 +256,10 @@ spec:
{{- end }}
terminationGracePeriodSeconds: 310
volumes:
- name: secrets
secret:
secretName: "{{ .Values.secretName }}"
defaultMode: 0400
- name: tmp
emptyDir: {}
{{- if .Values.db.useCertificate }}
Expand Down