EST with postgresql based authentication #4856

fmarco76 · 2024-09-24T12:17:04Z

PKIPostgreSQLRealm class has been modified to avoid and change in DB. The DB has to be ready before configuring the realm. A new test working with this realm is included.

Additionally, the realm test on separate instance has been modified to authenticate the EST subsystem into the CA using a certificate.

fmarco76 · 2024-09-24T13:27:20Z

@edewata I'll wait #4854 and update the CI before merge.

edewata · 2024-09-24T15:23:56Z

@fmarco76 I've merged PR #4854. FYI, I'm planning to modify pkispawn so that it can create an SSL server cert for ACME automatically, and I'll modify the ACME test with a separate instance to use that automation, but I'll keep the original test with manual cert creation as a separate test (i.e. ACME with existing NSS database) to make sure it will support reinstallation or containers (where the NSS database already exists).

fmarco76 · 2024-09-24T18:27:34Z

@edewata I have updated the actions

edewata

It looks good in general, but please see my comments below.

edewata · 2024-09-24T20:26:55Z

.github/workflows/est-ds-realm-separate-test.yml

+          docker exec ca pki -n caadmin ca-cert-export --output-file estUser.crt $CERT_ID
+          docker exec ca pki nss-cert-import --cert estUser.crt estUser
+
+          docker exec ca pk12util -d /root/.dogtag/nssdb -o $SHARED/est_user.p12 -n estUser -W Secret.123


It might be possible to import the admin cert & key using pki pkcs12-cert-import --append into est_server.p12 so they can be imported by pkispawn.

https://github.com/dogtagpki/pki/wiki/PKI-PKCS12-Certificate-CLI

I was looking a way to easily append to the same p12 but I did not find it. This is way better, thanks!

edewata · 2024-09-24T20:30:54Z

.github/workflows/est-ds-realm-separate-test.yml

+          docker exec ca pki client-cert-request uid=estUser | tee output
+          REQUEST_ID=$(sed -n "s/^\s*Request ID:\s*\(\S*\)$/\1/p" output)
+
+          docker exec ca pki -n caadmin ca-cert-request-approve $REQUEST_ID --force | tee output
+          CERT_ID=$(sed -n "s/^\s*Certificate ID:\s*\(\S*\)$/\1/p" output)
+
+          docker exec ca pki -n caadmin ca-user-cert-add est-ra-1 --serial $CERT_ID
+          docker exec ca pki -n caadmin ca-cert-export --output-file estUser.crt $CERT_ID


It might be possible to use pki ca-cert-issue to issue the cert, then use pki ca-user-cert-add --input to assign the cert.

https://github.com/dogtagpki/pki/wiki/PKI-User-Certificate-CLI

edewata · 2024-09-24T21:21:20Z

base/server/src/main/java/com/netscape/cms/realm/PKIPostgreSQLRealm.java

-    public void setup() throws Exception {
-
-        logger.info("Setting up PostgreSQL realm");
-
-        String filename = "/usr/share/pki/acme/realm/postgresql/create.sql";
-        String content = new String(Files.readAllBytes(Paths.get(filename)));
-
-        String[] stats = content.split(";");
-        for (String sql : stats) {
-            sql = sql.trim();
-            if (StringUtils.isEmpty(sql)) continue;
-            logger.info("SQL: " + sql);
-
-            try (PreparedStatement ps = connection.prepareStatement(sql)) {
-                ps.executeUpdate();
-
-            } catch (SQLException e) {
-
-                // https://www.postgresql.org/docs/current/errcodes-appendix.html
-                String sqlState = e.getSQLState();
-
-                // If table already exists, ignore
-                if (!"42P07".equals(sqlState)) {
-
-                    logger.error("Unable to set up PostgreSQL realm: " + e.getMessage());
-                    logger.error("SQL state: " + sqlState);
-
-                    throw e;
-                }
-            }
-        }
-    }


Is it possible to keep this code? I think it was meant to make ACME easier to use, i.e. just run PostgreSQL and ACME, and it's ready to use (ACME doesn't require initial users). If necessary (e.g. for EST) the tables can be created first and this code will not alter the tables. We probably can add a new param in realm.conf to specify the path, for example:

create=/usr/share/pki/acme/realm/postgresql/create.sql

The problem of this code is that it check for the table and if not present then it will create the tables. If the DB is provided by the user and the table names do not match the new table should not be created. We can make this optional.

edewata

Thanks for the updates! I have a few more comments but I'll let you decide. Feel free to update/merge.

edewata · 2024-09-25T14:23:22Z

.github/workflows/est-ds-realm-separate-test.yml

+          CERT_ID=$(docker exec ca pki nss-cert-show estUser |sed -n "s/^\s*Serial Number:\s*\(\S*\)$/\1/p")
+          docker exec ca pki -n caadmin ca-user-cert-add est-ra-1 --serial $CERT_ID


I think we can use pki ca-user-cert-add --input so no need to parse the serial number.

https://github.com/dogtagpki/pki/wiki/PKI-User-Certificate-CLI

edewata · 2024-09-25T14:36:08Z

base/server/src/main/java/com/netscape/cms/realm/PKIPostgreSQLRealm.java

+
+        String createFile = info.getProperty("dbcreate.file");
+        if (createFile != null) {
+            try{
+                connect();
+                setup(createFile);
+            } catch (Exception e) {
+                throw new LifecycleException("DB creation failed. Creation file: " + createFile, e);
+            }
+        }
+


I think the setup() was originally called from connect() because there is no guarantee that containers will be started in certain orders. For example, sometimes the ACME container is started first, then the PostgreSQL container is added later or it was not running when ACME container is started. With the new code that probably won't work anymore. My suggestion is to return setup() to its original position, but we can keep the dbcreate.file param. What do you think?

I do not think this parameter should be used after the installation. If we have it in the init then at the server start the DB should be created and the parameter should be removed. This could be done during pkispawn by the user.

In a container environment there is no reason to provide a DB without table after the initial setup. Who should fill the table with the user? In any case it should be possible to define a startup order although it is not needed in this case.

Since the current implementation perform the check in the connect() I am reverting it and evaluate again when we will move to container.

Thanks! Yeah, ACME doesn't require any user unless we want to manage something remotely (which there isn't much that can be done anyway). EST seems to require a pre-existing realm, so the requirement is different. We can certainly revisit this requirement again.

The realm should not modify the user database but it has to be provided and configured in advance. However, if a file is provided it is used during the initialisation and not during the authentication.

Adding a new test to verify the connection between the EST subsystem and a realm user database handled with Postgresql. Additionally, the realm test on separate instance has been modified to authenticate the EST subsystem into the CA using a certificate.

Since DB could be not available before the connection the setup is moved back to the connection method.

sonarcloud · 2024-09-25T16:22:29Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

fmarco76 · 2024-09-25T16:40:49Z

@edewata Thanks!

fmarco76 requested a review from edewata September 24, 2024 12:17

fmarco76 force-pushed the EST_CI branch 2 times, most recently from b711e45 to 5f80007 Compare September 24, 2024 17:59

edewata reviewed Sep 24, 2024

View reviewed changes

fmarco76 force-pushed the EST_CI branch 4 times, most recently from f924e7e to a8301ca Compare September 25, 2024 12:57

fmarco76 requested a review from edewata September 25, 2024 13:13

edewata approved these changes Sep 25, 2024

View reviewed changes

fmarco76 added 3 commits September 25, 2024 17:29

Remove setup from Postgresql realm authentication

1e44d21

The realm should not modify the user database but it has to be provided and configured in advance. However, if a file is provided it is used during the initialisation and not during the authentication.

Revert DB setup to the connection method

09c403f

Since DB could be not available before the connection the setup is moved back to the connection method.

fmarco76 force-pushed the EST_CI branch from a8301ca to 09c403f Compare September 25, 2024 15:29

fmarco76 merged commit 5f28b99 into dogtagpki:master Sep 25, 2024
157 of 164 checks passed

fmarco76 deleted the EST_CI branch September 25, 2024 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EST with postgresql based authentication #4856

EST with postgresql based authentication #4856

fmarco76 commented Sep 24, 2024

fmarco76 commented Sep 24, 2024

edewata commented Sep 24, 2024

fmarco76 commented Sep 24, 2024

edewata left a comment

edewata Sep 24, 2024

fmarco76 Sep 25, 2024

edewata Sep 24, 2024

edewata Sep 24, 2024

fmarco76 Sep 25, 2024

edewata left a comment

edewata Sep 25, 2024

edewata Sep 25, 2024

fmarco76 Sep 25, 2024

edewata Sep 25, 2024

sonarcloud bot commented Sep 25, 2024

fmarco76 commented Sep 25, 2024

		CERT_ID=$(docker exec ca pki nss-cert-show estUser \|sed -n "s/^\sSerial Number:\s\(\S*\)$/\1/p")
		docker exec ca pki -n caadmin ca-user-cert-add est-ra-1 --serial $CERT_ID

EST with postgresql based authentication #4856

EST with postgresql based authentication #4856

Conversation

fmarco76 commented Sep 24, 2024

fmarco76 commented Sep 24, 2024

edewata commented Sep 24, 2024

fmarco76 commented Sep 24, 2024

edewata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edewata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 25, 2024

Quality Gate passed

fmarco76 commented Sep 25, 2024