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(db): inherit from scl image, enforce PG_ENCRYPT_KEY #129

Merged
merged 19 commits into from
Oct 30, 2023

Conversation

andrewazores
Copy link
Member

Welcome to Cryostat3! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Fixes: #128

@andrewazores andrewazores added feat New feature or request safe-to-test labels Oct 25, 2023
db/entrypoint.bash Outdated Show resolved Hide resolved
@andrewazores andrewazores force-pushed the database-require-encryption branch from 58a761a to abefe89 Compare October 25, 2023 21:27
ebaron
ebaron previously approved these changes Oct 25, 2023
@andrewazores
Copy link
Member Author

Ah, hang on. Now I need to figure out how to get Quarkus to set an encryption key when it spins up the database container for tests.

@andrewazores
Copy link
Member Author

I think this is ready for review again.

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

I discovered that there's an upstream CentOS Stream image of the RHEL 8 Postgres image. What do you think of using that in order to minimize differences?
https://quay.io/repository/sclorg/postgresql-15-c8s

@andrewazores
Copy link
Member Author

That container has an entrypoint /usr/bin/container-entrypoint which is a little odd:

$ cat /usr/bin/container-entrypoint
#!/bin/bash

exec "$@"

The container is built with that as the default entrypoint and the default CMD being run-postgresql. That's a reference to another script in the $PATH:

$ cat /usr/bin/run-postgresql
#!/bin/bash

export ENABLE_REPLICATION=${ENABLE_REPLICATION:-false}

set -eu
export_vars=$(cgroup-limits) ; export $export_vars

source "${CONTAINER_SCRIPTS_PATH}/common.sh"

set_pgdata

process_extending_files \
        "${APP_DATA}/src/postgresql-pre-start" \
        "${CONTAINER_SCRIPTS_PATH}/pre-start"

check_env_vars
generate_passwd_file
generate_postgresql_config

# Is this brand new data volume?
PG_INITIALIZED=false

if [ ! -f "$PGDATA/postgresql.conf" ]; then
  initialize_database
  PG_INITIALIZED=:
else
  try_pgupgrade
fi

# Use insanely large timeout (24h) to ensure that the potential recovery has
# enough time here to happen (unless liveness probe kills us).  Note that in
# case of server failure this command still exists immediately.
pg_ctl start -w --timeout 86400 -o "-h ''"

# This is just a pedantic safety measure (the timeout above is unlikely to
# happen), but `pt_ctl -w` is not reliable prior to PostgreSQL v10 where it
# returns exit_status=0 even if the server is still starting.  For more info
# see the issue#297 and
# https://www.postgresql.org/message-id/CAB7nPqSJs85wK9aknm%3D_jmS6GnH3SQBhpzKcqs8Qo2LhEg2etw%40mail.gmail.com
pg_isready

if $PG_INITIALIZED ; then
    process_extending_files \
        "${APP_DATA}/src/postgresql-init" \
        "${CONTAINER_SCRIPTS_PATH}/init"
    migrate_db
    create_users
fi

process_extending_files \
    "${APP_DATA}/src/postgresql-start" \
    "${CONTAINER_SCRIPTS_PATH}/start"

pg_ctl stop

unset_env_vars
echo "Starting server..."
exec postgres "$@"

The CONTAINER_SCRIPTS_PATH is /usr/share/container-scripts, so maybe that path is where the script for enabling the pgcrypto extension can go?

@andrewazores
Copy link
Member Author

andrewazores commented Oct 27, 2023

/usr/share/container-scripts/postgresql/README.md:

## Extending Image

You can extend this image in Openshift using the `Source` build strategy or via the standalone [source-to-image](https://github.com/openshift/source-to-image) application (where available). For this example, assume that you are using the `` image, available via `postgresql:15` imagestream tag in Openshift.

To build a customized image `new-postgresql` with configuration from `https://github.com/sclorg/postgresql-container/tree/master/examples/extending-image`, run:

$ oc new-app postgresql:15~https://github.com/sclorg/postgresql-container.git \
  --name new-postgresql \
  --context-dir examples/extending-image/ \
  -e POSTGRESQL_USER=user \
  -e POSTGRESQL_DATABASE=db \
  -e POSTGRESQL_PASSWORD=password

or via `s2i`:

$ s2i build --context-dir examples/extending-image/ https://github.com/sclorg/postgresql-container.git  new-postgresql

The directory passed to Openshift should contain one or more of the following directories:

##### `postgresql-pre-start/`

This directory should contain `*.sh` files that will be sourced during the early start of the container. At this point, there is no PostgreSQL daemon running in the background.

##### `postgresql-cfg/`

Configuration files (`*.conf`) contained in this directory will be included at the end of the image's postgresql.conf file.

##### `postgresql-init/`

This directory should contain shell scripts (`*.sh`) that are sourced when the database is freshly initialized (after a successful initdb run, which makes the data directory non-empty). At the time of sourcing these scripts, the local PostgreSQL server is running. For re-deployment scenarios with a persistent data directory, the scripts are not sourced (no-op).

##### `postgresql-start/`

This directory has the same semantics as `postgresql-init/`, except that these scripts are always sourced (after `postgresql-init/` scripts, if they exist).

---

During the s2i build, all provided files are copied into the `/opt/app-root/src`
directory in the new image. Only one file with the same name can be used for customization, and user-provided files take precedence over default files in `/usr/share/container-scripts/`. This means that it is possible to overwrite the default files.

So I guess I can create a shell script to drop into the init container script directory, which connects to the freshly-initialized database and executes the .sql snippet to enable the crypto extension.

@ebaron
Copy link
Member

ebaron commented Oct 27, 2023

I think it needs to be added to $APP_ROOT/src/postgresql-init/:
https://github.com/sclorg/postgresql-container/tree/master/examples/enable-extension

@andrewazores
Copy link
Member Author

Nice, that looks easy enough. I think it's still going to have a customized entrypoint that enforces the encryption key being set first, then hook back in to the usual entrypoint. WDYT?

@ebaron
Copy link
Member

ebaron commented Oct 27, 2023

Nice, that looks easy enough. I think it's still going to have a customized entrypoint that enforces the encryption key being set first, then hook back in to the usual entrypoint. WDYT?

I think so.

@andrewazores andrewazores changed the title feat(db): set custom entrypoint script that enforces $PG_ENCRYPT_KEY feat(db): inherit from scl image, enforce PG_ENCRYPT_KEY Oct 27, 2023
@andrewazores
Copy link
Member Author

db_1        | 2023-10-27 18:52:26.739 UTC [80] ERROR:  function pgp_sym_decrypt(bytea, text) does not exist at character 37
db_1        | 2023-10-27 18:52:26.739 UTC [80] HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
db_1        | 2023-10-27 18:52:26.739 UTC [80] STATEMENT:  select c1_0.id,c1_0.matchExpression,pgp_sym_decrypt(c1_0.password, current_setting('encrypt.key')),pgp_sym_decrypt(c1_0.username, current_setting('encrypt.key')) from Credential c1_0
cryostat_1  | Oct 27, 2023 6:52:26 PM org.hibernate.engine.jdbc.spi.SqlExceptionHelper logExceptions
cryostat_1  | WARN: SQL Error: 0, SQLState: 42883
cryostat_1  | Oct 27, 2023 6:52:26 PM org.hibernate.engine.jdbc.spi.SqlExceptionHelper logExceptions
cryostat_1  | ERROR: ERROR: function pgp_sym_decrypt(bytea, text) does not exist
cryostat_1  |   Hint: No function matches the given name and argument types. You might need to add explicit type casts.
cryostat_1  |   Position: 37
cryostat_1  | Oct 27, 2023 6:52:26 PM io.quarkus.vertx.http.runtime.QuarkusErrorHandler handle
cryostat_1  | ERROR: HTTP Request to /api/v2.2/credentials failed, error id: b0d099cd-62ec-4505-9c5b-0d832de69ce2-1
cryostat_1  | org.hibernate.exception.SQLGrammarException: JDBC exception executing SQL [select c1_0.id,c1_0.matchExpression,pgp_sym_decrypt(c1_0.password, current_setting('encrypt.key')),pgp_sym_decrypt(c1_0.username, current_setting('encrypt.key')) from Credential c1_0] [ERROR: function pgp_sym_decrypt(bytea, text) does not exist
cryostat_1  |   Hint: No function matches the given name and argument types. You might need to add explicit type casts.
cryostat_1  |   Position: 37] [n/a]
cryostat_1  | 	at org.hibernate.exception.internal.SQLStateConversionDelegate.convert(SQLStateConversionDelegate.java:89)
cryostat_1  | 	at org.hibernate.exception.internal.StandardSQLExceptionConverter.convert(StandardSQLExceptionConverter.java:56)
cryostat_1  | 	at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:108)
cryostat_1  | 	at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:94)
cryostat_1  | 	at org.hibernate.sql.results.jdbc.internal.DeferredResultSetAccess.executeQuery(DeferredResultSetAccess.java:257)
cryostat_1  | 	at org.hibernate.sql.results.jdbc.internal.DeferredResultSetAccess.getResultSet(DeferredResultSetAccess.java:163)
cryostat_1  | 	at org.hibernate.sql.results.jdbc.internal.JdbcValuesResultSetImpl.advanceNext(JdbcValuesResultSetImpl.java:254)
cryostat_1  | 	at org.hibernate.sql.results.jdbc.internal.JdbcValuesResultSetImpl.processNext(JdbcValuesResultSetImpl.java:134)
cryostat_1  | 	at org.hibernate.sql.results.jdbc.internal.AbstractJdbcValues.next(AbstractJdbcValues.java:19)
cryostat_1  | 	at org.hibernate.sql.results.internal.RowProcessingStateStandardImpl.next(RowProcessingStateStandardImpl.java:66)
cryostat_1  | 	at org.hibernate.sql.results.spi.ListResultsConsumer.consume(ListResultsConsumer.java:198)
cryostat_1  | 	at org.hibernate.sql.results.spi.ListResultsConsumer.consume(ListResultsConsumer.java:33)
cryostat_1  | 	at org.hibernate.sql.exec.internal.JdbcSelectExecutorStandardImpl.doExecuteQuery(JdbcSelectExecutorStandardImpl.java:361)
cryostat_1  | 	at org.hibernate.sql.exec.internal.JdbcSelectExecutorStandardImpl.executeQuery(JdbcSelectExecutorStandardImpl.java:168)
cryostat_1  | 	at org.hibernate.sql.exec.internal.JdbcSelectExecutorStandardImpl.list(JdbcSelectExecutorStandardImpl.java:93)
cryostat_1  | 	at org.hibernate.sql.exec.spi.JdbcSelectExecutor.list(JdbcSelectExecutor.java:31)
cryostat_1  | 	at org.hibernate.query.sqm.internal.ConcreteSqmSelectQueryPlan.lambda$new$0(ConcreteSqmSelectQueryPlan.java:110)
cryostat_1  | 	at org.hibernate.query.sqm.internal.ConcreteSqmSelectQueryPlan.withCacheableSqmInterpretation(ConcreteSqmSelectQueryPlan.java:303)
cryostat_1  | 	at org.hibernate.query.sqm.internal.ConcreteSqmSelectQueryPlan.performList(ConcreteSqmSelectQueryPlan.java:244)
cryostat_1  | 	at org.hibernate.query.sqm.internal.QuerySqmImpl.doList(QuerySqmImpl.java:518)
cryostat_1  | 	at org.hibernate.query.spi.AbstractSelectionQuery.list(AbstractSelectionQuery.java:367)
cryostat_1  | 	at org.hibernate.query.Query.getResultList(Query.java:119)
cryostat_1  | 	at io.quarkus.hibernate.orm.panache.common.runtime.CommonPanacheQueryImpl.list(CommonPanacheQueryImpl.java:280)
cryostat_1  | 	at io.quarkus.hibernate.orm.panache.runtime.PanacheQueryImpl.list(PanacheQueryImpl.java:149)
cryostat_1  | 	at io.quarkus.hibernate.orm.panache.runtime.JpaOperations.list(JpaOperations.java:24)
cryostat_1  | 	at io.quarkus.hibernate.orm.panache.runtime.JpaOperations.list(JpaOperations.java:10)
cryostat_1  | 	at io.quarkus.hibernate.orm.panache.common.runtime.AbstractJpaOperations.listAll(AbstractJpaOperations.java:305)
cryostat_1  | 	at io.cryostat.credentials.Credential.listAll(Credential.java)
cryostat_1  | 	at io.cryostat.credentials.Credentials.list(Credentials.java:53)
cryostat_1  | 	at io.cryostat.credentials.Credentials_Subclass.list$$superforward(Unknown Source)
cryostat_1  | 	at io.cryostat.credentials.Credentials_Subclass$$function$$2.apply(Unknown Source)
cryostat_1  | 	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:73)
cryostat_1  | 	at io.quarkus.arc.impl.AroundInvokeInvocationContext$NextAroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:97)
cryostat_1  | 	at io.quarkus.security.runtime.interceptor.SecurityHandler.handle(SecurityHandler.java:27)
cryostat_1  | 	at io.quarkus.security.runtime.interceptor.RolesAllowedInterceptor.intercept(RolesAllowedInterceptor.java:29)
cryostat_1  | 	at io.quarkus.security.runtime.interceptor.RolesAllowedInterceptor_Bean.intercept(Unknown Source)
cryostat_1  | 	at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
cryostat_1  | 	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:70)
cryostat_1  | 	at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:62)
cryostat_1  | 	at io.quarkus.resteasy.reactive.server.runtime.StandardSecurityCheckInterceptor.intercept(StandardSecurityCheckInterceptor.java:44)
cryostat_1  | 	at io.quarkus.resteasy.reactive.server.runtime.StandardSecurityCheckInterceptor_RolesAllowedInterceptor_Bean.intercept(Unknown Source)
cryostat_1  | 	at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
cryostat_1  | 	at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:30)
cryostat_1  | 	at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:27)
cryostat_1  | 	at io.cryostat.credentials.Credentials_Subclass.list(Unknown Source)
cryostat_1  | 	at io.cryostat.credentials.Credentials$quarkusrestinvoker$list_ce6c13b83885b2c6f02955ea580a8e8e537fe383.invoke(Unknown Source)
cryostat_1  | 	at org.jboss.resteasy.reactive.server.handlers.InvocationHandler.handle(InvocationHandler.java:29)
cryostat_1  | 	at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.invokeHandler(QuarkusResteasyReactiveRequestContext.java:141)
cryostat_1  | 	at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:145)
cryostat_1  | 	at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:576)
cryostat_1  | 	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2513)
cryostat_1  | 	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1538)
cryostat_1  | 	at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:29)
cryostat_1  | 	at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:29)
cryostat_1  | 	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
cryostat_1  | 	at java.base/java.lang.Thread.run(Thread.java:840)
cryostat_1  | Caused by: org.postgresql.util.PSQLException: ERROR: function pgp_sym_decrypt(bytea, text) does not exist
cryostat_1  |   Hint: No function matches the given name and argument types. You might need to add explicit type casts.
cryostat_1  |   Position: 37
cryostat_1  | 	at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2713)
cryostat_1  | 	at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2401)
cryostat_1  | 	at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:368)
cryostat_1  | 	at org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:498)
cryostat_1  | 	at org.postgresql.jdbc.PgStatement.execute(PgStatement.java:415)
cryostat_1  | 	at org.postgresql.jdbc.PgPreparedStatement.executeWithFlags(PgPreparedStatement.java:190)
cryostat_1  | 	at org.postgresql.jdbc.PgPreparedStatement.executeQuery(PgPreparedStatement.java:134)
cryostat_1  | 	at io.agroal.pool.wrapper.PreparedStatementWrapper.executeQuery(PreparedStatementWrapper.java:78)
cryostat_1  | 	at org.hibernate.sql.results.jdbc.internal.DeferredResultSetAccess.executeQuery(DeferredResultSetAccess.java:239)
cryostat_1  | 	... 51 more

@andrewazores andrewazores marked this pull request as draft October 27, 2023 19:08
@andrewazores andrewazores marked this pull request as ready for review October 27, 2023 20:12
@andrewazores
Copy link
Member Author

Okay, I don't know if what I did there is best practice, but it seems reasonable. I got it from here: https://stackoverflow.com/questions/12986368/installing-postgresql-extension-to-all-schemas

I added a super-basic test that makes sure that the credentials table can be queried, so that exercises the database connection and makes sure the pgcrypto stuff is actually available.

@andrewazores andrewazores merged commit 3e488fe into cryostatio:main Oct 30, 2023
8 checks passed
@andrewazores andrewazores deleted the database-require-encryption branch October 30, 2023 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request safe-to-test
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Database should require PG_ENCRYPT_KEY and bail out if none provided
2 participants