Check whether ffdhe2048.pem exists before writing #38

govekk · 2023-09-13T18:16:41Z

omero-certificates 0.3.0 always attempts to write ffdhe2048.pem to the OMERO certs directory, which breaks read-only deployments of OMERO.server. The solution is to check whether ffdhe2048.pem already exists in that directory, and to write it only if it doesn't exist.

sbesson · 2023-09-13T18:34:35Z

Thanks for your contribution @govekk. The read-only scenario had indeed not been included in the testing of #36 and requires some attention. I will take a look at this tomorrow.

In the meantime, would you be able to return an executed CLA as per the instructions in https://ome-contributing.readthedocs.io/en/latest/cla.html so that this PR can be merged by the OME team once reviewed and approved?

sbesson

I successfully tested the upgrade of a IDR testing environment using read-only OEMRO server to omero-certificates 0.3.0 followed by a server restart (which also invokes omero certificates)

[sbesson@test120-omeroreadonly-2 ~]$ sudo /opt/omero/server/venv3/bin/pip install -U omero-certificates
Requirement already satisfied: omero-certificates in /opt/omero/server/venv3/lib/python3.6/site-packages (0.2.0)
Collecting omero-certificates
  Downloading omero_certificates-0.3.0-py3-none-any.whl (13 kB)
Collecting distro==1.8.0
  Downloading distro-1.8.0-py3-none-any.whl (20 kB)
Requirement already satisfied: omero-py>=5.6.0 in /opt/omero/server/venv3/lib/python3.6/site-packages (from omero-certificates) (5.15.0)
Requirement already satisfied: Pillow in /opt/omero/server/venv3/lib/python3.6/site-packages (from omero-py>=5.6.0->omero-certificates) (8.4.0)
Requirement already satisfied: zeroc-ice<3.7,>=3.6.5 in /opt/omero/server/venv3/lib/python3.6/site-packages (from omero-py>=5.6.0->omero-certificates) (3.6.5)
Requirement already satisfied: appdirs in /opt/omero/server/venv3/lib/python3.6/site-packages (from omero-py>=5.6.0->omero-certificates) (1.4.4)
Requirement already satisfied: requests in /opt/omero/server/venv3/lib/python3.6/site-packages (from omero-py>=5.6.0->omero-certificates) (2.27.1)
Requirement already satisfied: PyYAML in /opt/omero/server/venv3/lib/python3.6/site-packages (from omero-py>=5.6.0->omero-certificates) (6.0.1)
Requirement already satisfied: numpy in /opt/omero/server/venv3/lib/python3.6/site-packages (from omero-py>=5.6.0->omero-certificates) (1.19.5)
Requirement already satisfied: future in /opt/omero/server/venv3/lib/python3.6/site-packages (from omero-py>=5.6.0->omero-certificates) (0.18.3)
Requirement already satisfied: charset-normalizer~=2.0.0 in /opt/omero/server/venv3/lib/python3.6/site-packages (from requests->omero-py>=5.6.0->omero-certificates) (2.0.12)
Requirement already satisfied: certifi>=2017.4.17 in /opt/omero/server/venv3/lib/python3.6/site-packages (from requests->omero-py>=5.6.0->omero-certificates) (2023.7.22)
Requirement already satisfied: urllib3<1.27,>=1.21.1 in /opt/omero/server/venv3/lib/python3.6/site-packages (from requests->omero-py>=5.6.0->omero-certificates) (1.26.16)
Requirement already satisfied: idna<4,>=2.5 in /opt/omero/server/venv3/lib/python3.6/site-packages (from requests->omero-py>=5.6.0->omero-certificates) (3.4)
Installing collected packages: distro, omero-certificates
  Attempting uninstall: omero-certificates
    Found existing installation: omero-certificates 0.2.0
    Uninstalling omero-certificates-0.2.0:
      Successfully uninstalled omero-certificates-0.2.0
Successfully installed distro-1.8.0 omero-certificates-0.3.0
[sbesson@test120-omeroreadonly-2 ~]$ sudo -u omero-server /opt/omero/server/OMERO.server/bin/omero certificates -v
WARNING:omero_certificates.certificates:Your Linux distribution has been detected as RHEL 7 which will reach end of life in June 2024.  TLS 1.3 cannot be enabled and upgrading is recommended.
See https://www.openmicroscopy.org/2023/07/24/linux-distributions.html for more information.
INFO:omero_certificates.certificates:Setting omero.glacier2.IceSSL.DH.2048=ffdhe2048.pem
INFO:omero_certificates.certificates:Executing: openssl version
OpenSSL 1.0.2k-fips  26 Jan 2017
INFO:omero_certificates.certificates:Using existing key: /data/OMERO/certs/server.key
INFO:omero_certificates.certificates:Creating self-signed certificate: /data/OMERO/certs/server.pem
INFO:omero_certificates.certificates:Executing: openssl req -new -x509 -subj /L=OMERO/O=OMERO.server/CN=localhost -days 365 -key /data/OMERO/certs/server.key -out /data/OMERO/certs/server.pem -extensions v3_ca
INFO:omero_certificates.certificates:Creating PKCS12 bundle: /data/OMERO/certs/server.p12
INFO:omero_certificates.certificates:Executing: openssl pkcs12 -export -out /data/OMERO/certs/server.p12 -inkey /data/OMERO/certs/server.key -in /data/OMERO/certs/server.pem -name server -password pass:secret
certificates created: /data/OMERO/certs/server.pem /data/OMERO/certs/server.p12
[sbesson@test120-omeroreadonly-2 ~]$ sudo systemctl restart omero-server
[sbesson@test120-omeroreadonly-2 ~]$ ls -alh /data/OMERO/certs/
total 16K
drwxr-xr-x. 2 omero-server omero-server   81 Sep 14 08:25 .
drwxr-xr-x. 4 omero-server root          157 Sep  6 09:24 ..
-rw-r--r--. 1 omero-server omero-server  423 Sep 14 08:27 ffdhe2048.pem
-rw-r--r--. 1 omero-server omero-server 1.7K Sep  6 09:24 server.key
-rw-r--r--. 1 omero-server omero-server 2.5K Sep 14 08:27 server.p12
-rw-r--r--. 1 omero-server omero-server 1.2K Sep 14 08:27 server.pem

Could you give more details on the error you are seeing in your environment? As you can see from the timestamps above, the plugin already expects the OMERO certificates directory is writeable as it regenerates the public certificate and combined keys on every invocation of the command.

Separately from the consideration above, I agree there is no requirement to rewrite the same content on every invocation of omero certificates. The proposed change applies the same strategy used for the private key. Since the pem file is effectively managed by the plugin,I would also propose to read its contents if it exists and validate it is identical to FFDHE2048_PEM.

govekk · 2023-09-14T20:13:54Z

Thank you for the details - this seems like a difference in how we deploy read-only OMERO servers. The current read-only server that we run at The Jackson Laboratory mounts the full OMERO directory (including all subfolders) as read-only. A separate read-write server is started first to create the system files and certificates. On startup of the read-only server, there are a few warnings from the certificate creation about the read-only file system, but until now they have not stopped the server from starting.

Using omero-certificates 0.2.0:

OpenSSL 1.1.1n  15 Mar 2022
Can't open /OMERO/certs/server.pem for writing, Read-only file system
140289759524032:error:0200101E:system library:fopen:Read-only file system:../crypto/bio/bss_file.c:69:fopen('/OMERO/certs/server.pem','w')
140289759524032:error:2006D002:BIO routines:BIO_new_file:system lib:../crypto/bio/bss_file.c:78:
pkcs12: Can't open "/OMERO/certs/server.p12" for writing, Read-only file system
certificates created: /OMERO/certs/server.pem /OMERO/certs/server.p12

(after which the startup scripts continue running, and the server starts successfully)

Using omero-certificates 0.3.0, the ffdhe2048.pem write errors in a way that halts the startup scripts:

OpenSSL 1.1.1n  15 Mar 2022
Traceback (most recent call last):
  File "/opt/omero/server/server_venv/bin/omero", line 8, in <module>
    sys.exit(main())
  File "/opt/omero/server/server_venv/lib/python3.7/site-packages/omero/main.py", line 126, in main
    rv = omero.cli.argv()
  File "/opt/omero/server/server_venv/lib/python3.7/site-packages/omero/cli.py", line 1787, in argv
    cli.invoke(args[1:])
  File "/opt/omero/server/server_venv/lib/python3.7/site-packages/omero/cli.py", line 1225, in invoke
    stop = self.onecmd(line, previous_args)
  File "/opt/omero/server/server_venv/lib/python3.7/site-packages/omero/cli.py", line 1302, in onecmd
    self.execute(line, previous_args)
  File "/opt/omero/server/server_venv/lib/python3.7/site-packages/omero/cli.py", line 1384, in execute
    args.func(args)
  File "/opt/omero/server/server_venv/lib/python3.7/site-packages/omero_certificates/cli.py", line 33, in certificates
    m = create_certificates(omerodir)
  File "/opt/omero/server/server_venv/lib/python3.7/site-packages/omero_certificates/certificates.py", line 135, in create_certificates
    with open(grouppath, "w") as pem:
OSError: [Errno 30] Read-only file system: '/OMERO/certs/ffdhe2048.pem'

We could catch this externally to error gracefully, but it made sense to mimic the strategy used for the private key.

Thank you for the suggestion on checking the content of ffdhe2048.pem. I agree that is a good idea, and I can implement it if you are willing to continue moving forward with this change.

sbesson · 2023-09-15T07:45:29Z

Interesting, reading about your deployment and looking at the stack trace above, it looks like your use case has been benefiting from the lenient behavior of the current implementation which executes the openssl command in a subprocess but will not raise an exception if the return code is non-zero.

Note this behavior might change in the future as we are looking into moving away from openssl and using a pure Python approach (see #27). We will need to keep the use case of the read-only certificates folder in mind and possibly treat certificates and combined keys like private keys and skip their creation if existing.

For the immediate use case, I am personally supportive of the approach proposed above i.e. check if the group PEM file exists and either generate it if it doesn't (in which case a writeable assumption is reasonable) or verify its content if it does.

sbesson

See discussion above. One minor suggestion to check the content of the PEM file on existence

govekk · 2023-11-02T15:47:53Z

Thank you for your patience - it took a bit to get permission to sign the CLA, but I sent that over to [email protected]. I just merged the requested change to check the content of the PEM file and overwrite if missing or different.

Thanks for the warning about the upcoming switch from OpenSSL to Python. We'll watch for it, but I appreciate you also taking into consideration the case of read-only certificates.

sbesson

Tested both in a full deployment as well as a local test environment using OMERODIR=/tmp/omero and omero.data.dir set to /tmp/omero/data

The initial execution of the script creates the certs directory and generates all its content as expected

(base) sbesson@Sebastiens-MacBook-Pro-3 omero-certificates % venv/bin/omero certificates -v 
INFO:omero_certificates.certificates:Executing: openssl version
OpenSSL 3.1.3 19 Sep 2023 (Library: OpenSSL 3.1.3 19 Sep 2023)
INFO:omero_certificates.certificates:Creating self-signed CA key: /tmp/omero/data/certs/server.key
INFO:omero_certificates.certificates:Executing: openssl genrsa -out /tmp/omero/data/certs/server.key 2048
INFO:omero_certificates.certificates:Creating self-signed certificate: /tmp/omero/data/certs/server.pem
INFO:omero_certificates.certificates:Executing: openssl req -new -x509 -subj /L=OMERO/O=OMERO.server/CN=localhost -days 365 -key /tmp/omero/data/certs/server.key -out /tmp/omero/data/certs/server.pem -extensions v3_ca
INFO:omero_certificates.certificates:Creating PKCS12 bundle: /tmp/omero/data/certs/server.p12
INFO:omero_certificates.certificates:Executing: openssl pkcs12 -export -out /tmp/omero/data/certs/server.p12 -inkey /tmp/omero/data/certs/server.key -in /tmp/omero/data/certs/server.pem -name server -password pass:secret
certificates created: /tmp/omero/data/certs/server.key /tmp/omero/data/certs/server.pem /tmp/omero/data/certs/server.p12
(base) sbesson@Sebastiens-MacBook-Pro-3 omero-certificates % ls -alh /tmp/omero/data/certs 
total 32
drwxr-xr-x  6 sbesson  wheel   192B  6 Nov 12:16 .
drwxr-xr-x  3 sbesson  wheel    96B  6 Nov 12:16 ..
-rw-r--r--  1 sbesson  wheel   423B  6 Nov 12:16 ffdhe2048.pem
-rw-------  1 sbesson  wheel   1.7K  6 Nov 12:16 server.key
-rw-------  1 sbesson  wheel   2.6K  6 Nov 12:16 server.p12
-rw-r--r--  1 sbesson  wheel   1.2K  6 Nov 12:16 server.pem

A re-execution of the same command correctly indicates the PEM file will be re-used. The timestamp of the underlying file confirms it is no longer ovewritten

(base) sbesson@Sebastiens-MacBook-Pro-3 omero-certificates % venv/bin/omero certificates -v 
INFO:omero_certificates.certificates:Executing: openssl version
OpenSSL 3.1.3 19 Sep 2023 (Library: OpenSSL 3.1.3 19 Sep 2023)
INFO:omero_certificates.certificates:Using existing ffdhe2048.pem
INFO:omero_certificates.certificates:Using existing key: /tmp/omero/data/certs/server.key
INFO:omero_certificates.certificates:Creating self-signed certificate: /tmp/omero/data/certs/server.pem
INFO:omero_certificates.certificates:Executing: openssl req -new -x509 -subj /L=OMERO/O=OMERO.server/CN=localhost -days 365 -key /tmp/omero/data/certs/server.key -out /tmp/omero/data/certs/server.pem -extensions v3_ca
INFO:omero_certificates.certificates:Creating PKCS12 bundle: /tmp/omero/data/certs/server.p12
INFO:omero_certificates.certificates:Executing: openssl pkcs12 -export -out /tmp/omero/data/certs/server.p12 -inkey /tmp/omero/data/certs/server.key -in /tmp/omero/data/certs/server.pem -name server -password pass:secret
certificates created: /tmp/omero/data/certs/server.pem /tmp/omero/data/certs/server.p12
(base) sbesson@Sebastiens-MacBook-Pro-3 omero-certificates % ls -alh /tmp/omero/data/certs  
total 32
drwxr-xr-x  6 sbesson  wheel   192B  6 Nov 12:16 .
drwxr-xr-x  3 sbesson  wheel    96B  6 Nov 12:16 ..
-rw-r--r--  1 sbesson  wheel   423B  6 Nov 12:16 ffdhe2048.pem
-rw-------  1 sbesson  wheel   1.7K  6 Nov 12:16 server.key
-rw-------  1 sbesson  wheel   2.6K  6 Nov 12:17 server.p12
-rw-r--r--  1 sbesson  wheel   1.2K  6 Nov 12:17 server.pem

After modifying the content of the PEM file , a new execution of omero certificates results in the PEM file being re-written with the pre-defined DH group as expected. Note the log message suggests the file will be re-used:

(base) sbesson@Sebastiens-MacBook-Pro-3 omero-certificates % echo "foo" > /tmp/omero/data/certs/ffdhe2048.pem 
(base) sbesson@Sebastiens-MacBook-Pro-3 omero-certificates % venv/bin/omero certificates -v                  
INFO:omero_certificates.certificates:Executing: openssl version
OpenSSL 3.1.3 19 Sep 2023 (Library: OpenSSL 3.1.3 19 Sep 2023)
INFO:omero_certificates.certificates:Using existing ffdhe2048.pem
INFO:omero_certificates.certificates:Using existing key: /tmp/omero/data/certs/server.key
INFO:omero_certificates.certificates:Creating self-signed certificate: /tmp/omero/data/certs/server.pem
INFO:omero_certificates.certificates:Executing: openssl req -new -x509 -subj /L=OMERO/O=OMERO.server/CN=localhost -days 365 -key /tmp/omero/data/certs/server.key -out /tmp/omero/data/certs/server.pem -extensions v3_ca
INFO:omero_certificates.certificates:Creating PKCS12 bundle: /tmp/omero/data/certs/server.p12
INFO:omero_certificates.certificates:Executing: openssl pkcs12 -export -out /tmp/omero/data/certs/server.p12 -inkey /tmp/omero/data/certs/server.key -in /tmp/omero/data/certs/server.pem -name server -password pass:secret
certificates created: /tmp/omero/data/certs/server.pem /tmp/omero/data/certs/server.p12
(base) sbesson@Sebastiens-MacBook-Pro-3 omero-certificates % ls -alh /tmp/omero/data/certs                   
total 32
drwxr-xr-x  6 sbesson  wheel   192B  6 Nov 12:16 .
drwxr-xr-x  3 sbesson  wheel    96B  6 Nov 12:16 ..
-rw-r--r--  1 sbesson  wheel   423B  6 Nov 12:17 ffdhe2048.pem
-rw-------  1 sbesson  wheel   1.7K  6 Nov 12:16 server.key
-rw-------  1 sbesson  wheel   2.6K  6 Nov 12:17 server.p12
-rw-r--r--  1 sbesson  wheel   1.2K  6 Nov 12:17 server.pem

Overall, the new logic works as expected. Once the logging statements are updated to match the operation performed on disk, this should be ready for inclusion in the next release of the plugin

sbesson · 2023-11-06T09:40:27Z

omero_certificates/certificates.py

-        pem.write(FFDHE2048_PEM)
+    pem_exists = False
+    if os.path.exists(grouppath):
+        log.info("Using existing ffdhe2048.pem")


As shown during the testing, this statement should be moved before pem_exists = True. In the unlikely case where the PEM content is not identical to the pre-defined group, this plugin should overwrite it and should not log that the existing PEM file will be used.

sbesson · 2023-11-06T12:23:29Z

omero_certificates/certificates.py

+                pem_exists = True
+    if not pem_exists:
+        with open(grouppath, "w") as pem:
+            pem.write(FFDHE2048_PEM)


As a mirror to the log statement discussed above about re-using the existing PEM file, I would propose to add a logging statement above similar to the ones below e.g.

log.info("Creating PEM file with pre-defined DH group: %s", pem)

sbesson · 2023-11-06T13:54:12Z

In addition to the review above, 8d27e4f should supplement the unit tests to cover the different scenarios of this PR. Feel free to cherry-pick this commit to this PR, otherwise I can open a follow-up PR to ensure we have coverage.

govekk · 2023-12-07T17:11:08Z

Thanks for the review - I've fixed the logging logic. The tests you linked worked well in my local environment, so I've included them as-is.

sbesson

Thanks @govekk, changes look great. The only remaining issue is the the diff of 31abfaf includes other contributions merged into origin/master and squashed into the commit

One way to address this is to merge origin/master into 688815a and then cherry-pick your last commit - see https://github.com/ome/omero-certificates/compare/master...sbesson:omero-certificates:ffdhe2048_check?expand=1 for the result. I'd propose to force push these commits to your branch so that we preserve the ownership of individual contributions. Is that fine with you? Afterwards, we should be good to get this released as 0.3.2

govekk · 2023-12-08T21:54:25Z

Ok, I think that's cleaned it up. Sorry for the authorship mess.

sbesson · 2024-01-17T13:45:43Z

@govekk sorry for the delay and thanks again for your contribution. This fix should now be available in omero-certificates 0.3.2 available from PyPI

Check whether ffdhe2048.pem exists before writing

83aebf5

sbesson self-requested a review September 13, 2023 18:30

sbesson reviewed Sep 14, 2023

View reviewed changes

sbesson mentioned this pull request Sep 14, 2023

Install more recent versions of OMERO.py ome/ansible-role-omero-prometheus-exporter#9

Merged

sbesson requested changes Oct 3, 2023

View reviewed changes

govekk added 2 commits November 2, 2023 11:05

Merge branch 'ome:master' into master

02bef39

checking that contents of pem match expectation (#1)

688815a

sbesson requested changes Nov 6, 2023

View reviewed changes

sbesson self-requested a review December 7, 2023 17:12

sbesson reviewed Dec 8, 2023

View reviewed changes

govekk added 3 commits December 8, 2023 14:30

Merge ome:master

f5e2d56

fixed logging

3360070

logging path on creation, not io wrapper

96b6d62

govekk force-pushed the master branch from 31abfaf to df0f4b7 Compare December 8, 2023 19:35

Expand certificate unit tests to cover existing PEM file scenarios

38d4959

govekk force-pushed the master branch from df0f4b7 to 38d4959 Compare December 8, 2023 21:40

sbesson approved these changes Dec 12, 2023

View reviewed changes

sbesson requested a review from jburel December 12, 2023 08:41

jburel approved these changes Jan 17, 2024

View reviewed changes

jburel merged commit f0aba79 into ome:master Jan 17, 2024
4 checks passed

sbesson mentioned this pull request Dec 16, 2024

Remove c7 support #54

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check whether ffdhe2048.pem exists before writing #38

Check whether ffdhe2048.pem exists before writing #38

govekk commented Sep 13, 2023

sbesson commented Sep 13, 2023

sbesson left a comment

govekk commented Sep 14, 2023

sbesson commented Sep 15, 2023

sbesson left a comment

govekk commented Nov 2, 2023

sbesson left a comment

sbesson Nov 6, 2023

sbesson Nov 6, 2023

sbesson commented Nov 6, 2023

govekk commented Dec 7, 2023

sbesson left a comment

govekk commented Dec 8, 2023

sbesson commented Jan 17, 2024

Check whether ffdhe2048.pem exists before writing #38

Check whether ffdhe2048.pem exists before writing #38

Conversation

govekk commented Sep 13, 2023

sbesson commented Sep 13, 2023

sbesson left a comment

Choose a reason for hiding this comment

govekk commented Sep 14, 2023

sbesson commented Sep 15, 2023

sbesson left a comment

Choose a reason for hiding this comment

govekk commented Nov 2, 2023

sbesson left a comment

Choose a reason for hiding this comment

sbesson Nov 6, 2023

Choose a reason for hiding this comment

sbesson Nov 6, 2023

Choose a reason for hiding this comment

sbesson commented Nov 6, 2023

govekk commented Dec 7, 2023

sbesson left a comment

Choose a reason for hiding this comment

govekk commented Dec 8, 2023

sbesson commented Jan 17, 2024