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

Does ssl-reload monitor keystore when it is a symlink? #8973

Closed
chetan777in opened this issue Nov 29, 2022 · 21 comments · Fixed by #9014
Closed

Does ssl-reload monitor keystore when it is a symlink? #8973

chetan777in opened this issue Nov 29, 2022 · 21 comments · Fixed by #9014
Labels

Comments

@chetan777in
Copy link

chetan777in commented Nov 29, 2022

jetty-9.4.49.v20220914 and 11.0.12

Java 11

Does ssl-reload monitor keystore when it is a symlink?

Have deployed my application which uses jetty in kubernetes pod. We are using cert-manger for certificate management.
Secret is mounted inside a pod as below:

drwxr-xr-x    2 root     root         140 Nov 29 09:38 ..2022_11_29_09_38_19.297914115
lrwxrwxrwx    1 root     root          31 Nov 29 09:38 ..data -> ..2022_11_29_09_38_19.297914115
lrwxrwxrwx    1 root     root          13 Nov 29 07:06 ca.crt -> ..data/ca.crt
lrwxrwxrwx    1 root     root          19 Nov 29 07:06 keystore.p12 -> ..data/keystore.p12
lrwxrwxrwx    1 root     root          14 Nov 29 07:06 tls.crt -> ..data/tls.crt
lrwxrwxrwx    1 root     root          14 Nov 29 07:06 tls.key -> ..data/tls.key
lrwxrwxrwx    1 root     root          21 Nov 29 07:06 truststore.p12 -> ..data/truststore.p12

When the certificates are renewed automatically by cert-manager , the target file for symlink keystore.p12 is changed , but even after that jetty is not picking up the updated certs. If keystore is not symlink , jetty is picking the updated certs.

@sbordet
Copy link
Contributor

sbordet commented Nov 29, 2022

In Jetty 9.4.x, by default, links are followed, so what's actually monitored would be ../data/keystore.p12.

Wait a second, do you really have a directory called ..data?
So it's not the .. as in "navigate to parent directory", it's actually part of the name of the directory?

Just out of curiosity, if you remove the .. from the beginning of the directory name, does it work?

@chetan777in
Copy link
Author

I can't edit anything in the mounted directory as it is read only and mounted/handled by kubernetes. This is how kubernetes mount secrets, so that when the secret is modified , updates are propogated inside pod.

..data directory is also a symlink

@sbordet
Copy link
Contributor

sbordet commented Nov 29, 2022

I see everything is owned by root, but I'll ask: is the file accessible/readable by Jetty when it starts?
Do you start Jetty with the root user?

@lachlan-roberts can you please check if the KeyStoreScanner has problems with directories that start with ..?

@chetan777in
Copy link
Author

I dont think permission is an issue, because certs are picked up when we start jetty first time , but when secret is updated i.e certificates are renewed , target files in symlinks gets updated but jetty does not pick the updated certificates.

@joakime
Copy link
Contributor

joakime commented Nov 29, 2022

Just a reminder that Jetty 9.4.x is now at End of Community Support.

@chetan777in
Copy link
Author

chetan777in commented Dec 1, 2022

I tired with jetty-home-11.0.12 and issue is same for keystore mounted as above. This is a very common scenario when deploying on kubernetes.

Even for normal symbolic links where .. is not involved in path , it detects the change but does not reload the certificates.

Here is the log snippet.

2022-11-30 13:56:01.339:DEBUG:oejx.XmlConfiguration:main: XML new org.eclipse.jetty.util.ssl.KeyStoreScanner
2022-11-30 13:56:01.340:DEBUG:oejus.KeyStoreScanner:main: Monitored Keystore File: /opt/jetty-home-11.0.12/frontend/etc/new/../keystore.p12
2022-11-30 13:56:01.343:DEBUG:oejuc.ContainerLifeCycle:main: KeyStoreScanner@45d84a20{STOPPED} added {Scanner@533bda92{STOPPED},AUTO}
2022-11-30 13:56:01.343:DEBUG:oejx.XmlConfiguration:main: XML KeyStoreScanner@45d84a20{STOPPED}.setScanInterval (1)
2022-11-30 13:56:01.344:DEBUG:oejuc.ContainerLifeCycle:main: Server@43301423{STOPPED}[11.0.12,sto=5000] added {KeyStoreScanner@45d84a20{STOPPED},AUTO}
2022-11-30 13:56:02.104:DEBUG:oejuc.AbstractLifeCycle:main: STARTING KeyStoreScanner@45d84a20{STOPPED}
2022-11-30 13:56:02.106:DEBUG:oejuc.AbstractLifeCycle:main: STARTED @1397ms KeyStoreScanner@45d84a20{STARTED}
2022-11-30 14:00:23.947:DEBUG:oejus.KeyStoreScanner:Scanner-0-1: changed /opt/jetty-home-11.0.12/frontend/etc/keystore.p12

I think issue #8786 fixes it , not sure if it is in jetty-home-11.0.12 ?

@lachlan-roberts
Copy link
Contributor

@lachlan-roberts can you please check if the KeyStoreScanner has problems with directories that start with ..?

I have tested KeyStoreScanner with directories starting with .. and it works fine.

@chetan777in when you say "the target file for symlink keystore.p12 is changed" do you mean you changed the symlink to point to a different target, or you replaced the target file of the symlink with the updated keystore?

@chetan777in
Copy link
Author

There are multiple symbolic links involved here.

drwxr-xr-x    2 root     root         140 Dec  1 07:19 ..2022_12_01_07_19_44.534825794
lrwxrwxrwx    1 root     root          31 Dec  1 07:19 ..data -> ..2022_12_01_07_19_44.534825794
lrwxrwxrwx    1 root     root          13 Dec  1 07:02 ca.crt -> ..data/ca.crt
lrwxrwxrwx    1 root     root          19 Dec  1 07:02 keystore.p12 -> ..data/keystore.p12
lrwxrwxrwx    1 root     root          14 Dec  1 07:02 tls.crt -> ..data/tls.crt
lrwxrwxrwx    1 root     root          14 Dec  1 07:02 tls.key -> ..data/tls.key
lrwxrwxrwx    1 root     root          21 Dec  1 07:02 truststore.p12 -> ..data/truststore.p12

The target file for keystore.p12 is ..data/keystore.p12 which is replaced by cert-manager when certificates are updated with the same name.
Please note that ..data directory is also a symlink to ..2022_12_01_07_19_44.534825794 . The name of this ..2022_12_01_07_19_44.534825794 directory is changed everytime certificates are updated to the time when certificates are updated.

Also for enabling ssl-reload module .I have enabled module by adding ssl-reload.ini into start.d folder . Is there any other config required ?

@chetan777in
Copy link
Author

chetan777in commented Dec 1, 2022

readlink resolves keystore.p12 symlink as below as there are recursive symlinks

frontend/etc/ssl# readlink -f keystore.p12
/opt/jetty-distribution-9.4.49.v20220914/frontend/etc/ssl/..2022_12_01_10_37_39.477917324/keystore.p12

@lachlan-roberts
Copy link
Contributor

This line of the logs is concerning to me

2022-11-30 13:56:01.340:DEBUG:oejus.KeyStoreScanner:main: Monitored Keystore File: /opt/jetty-home-11.0.12/frontend/etc/new/../keystore.p12

The alias is supposed to be resolved in the monitored file before this point so it shouldn't have the .. in the path.

There was a refactor for the alias resolution for Jetty 12 which I think may need to be backported to fix this.
see #8734

What is your keystore path set to for you SSL configuration?

@chetan777in
Copy link
Author

Since Jetty 9.x is out of community support , I tried with jetty-home-11.0.12.

Below are logs with keystore.p12 mounted(as a Kubernetes secret) as shown above.

2022-12-05 07:06:12.767:DEBUG:oejx.XmlConfiguration:main: XML new org.eclipse.jetty.util.ssl.KeyStoreScanner
2022-12-05 07:06:12.768:DEBUG:oejus.KeyStoreScanner:main: Monitored Keystore File: /opt/jetty-home-11.0.12/frontend/etc/ssl/..data/keystore.p12
2022-12-05 07:06:12.770:DEBUG:oejuc.ContainerLifeCycle:main: KeyStoreScanner@45d84a20{STOPPED} added {Scanner@533bda92{STOPPED},AUTO}
2022-12-05 07:06:12.771:DEBUG:oejx.XmlConfiguration:main: XML KeyStoreScanner@45d84a20{STOPPED}.setScanInterval (1)
2022-12-05 07:06:12.771:DEBUG:oejuc.ContainerLifeCycle:main: Server@43301423{STOPPED}[11.0.12,sto=5000] added {KeyStoreScanner@45d84a20{STOPPED},AUTO}
2022-12-05 07:06:13.514:DEBUG:oejuc.AbstractLifeCycle:main: STARTING KeyStoreScanner@45d84a20{STOPPED}
2022-12-05 07:06:13.515:DEBUG:oejuc.AbstractLifeCycle:main: STARTED @1346ms KeyStoreScanner@45d84a20{STARTED}
2022-12-05 07:10:01.962:DEBUG:oejus.KeyStoreScanner:Scanner-0-1: removed /opt/jetty-home-11.0.12/frontend/etc/ssl/..2022_12_05_06_59_05.185701460/ca.crt
2022-12-05 07:10:01.962:DEBUG:oejus.KeyStoreScanner:Scanner-0-1: removed /opt/jetty-home-11.0.12/frontend/etc/ssl/..2022_12_05_06_59_05.185701460/keystore.p12
2022-12-05 07:10:01.962:DEBUG:oejus.KeyStoreScanner:Scanner-0-1: removed /opt/jetty-home-11.0.12/frontend/etc/ssl/..2022_12_05_06_59_05.185701460/truststore.p12
2022-12-05 07:10:01.962:DEBUG:oejus.KeyStoreScanner:Scanner-0-1: removed /opt/jetty-home-11.0.12/frontend/etc/ssl/..2022_12_05_06_59_05.185701460/tls.crt
2022-12-05 07:10:01.962:DEBUG:oejus.KeyStoreScanner:Scanner-0-1: removed /opt/jetty-home-11.0.12/frontend/etc/ssl/..2022_12_05_06_59_05.185701460/tls.key

Keystore path set in {jetty-base}/etc/jetty-ssl-context.xml is : etc/ssl/keystore.p12

It does not seem to detect that there is change in keystore file. Can you please try with above file/directory structure for keystore.p12 with symlinks and confirm it works at your end ?

I see similar behavior in latest jetty 9.x and jetty 11.x for this case.

@joakime
Copy link
Contributor

joakime commented Dec 5, 2022

Jetty 11.0.12 does not have the fix from #8786
Can you try 11.0.13 (which is in the process of being tested for a release) instead?

https://oss.sonatype.org/content/groups/jetty-with-staging/org/eclipse/jetty/jetty-home/11.0.13/jetty-home-11.0.13.tar.gz

@chetan777in
Copy link
Author

This does not seem to work in jetty 11.0.13 as well.

Log shows same behavior as in previous jetty versions.
2022-12-06 08:33:34.270:DEBUG:oejus.KeyStoreScanner:main: Monitored Keystore File: /opt/jetty-home-11.0.13/frontend/etc/ssl/..data/keystore.p12

The keystore.p12 symlink does not seem to resolve to its final target which would be something like this:
/opt/jetty-home-11.0.13/frontend/etc/ssl/..2022_12_06_10_03_25.684878971/keystore.p12

I have set jetty.sslContext.reload.followLinks=true in start.d/ssl-reload.ini

@joakime
Copy link
Contributor

joakime commented Dec 6, 2022

Am I right in seeing that this is doubly sym-linked?

drwxr-xr-x    2 root     root         140 Dec  1 07:19 ..2022_12_01_07_19_44.534825794
lrwxrwxrwx    1 root     root          31 Dec  1 07:19 ..data -> ..2022_12_01_07_19_44.534825794
lrwxrwxrwx    1 root     root          13 Dec  1 07:02 ca.crt -> ..data/ca.crt
lrwxrwxrwx    1 root     root          19 Dec  1 07:02 keystore.p12 -> ..data/keystore.p12
lrwxrwxrwx    1 root     root          14 Dec  1 07:02 tls.crt -> ..data/tls.crt
lrwxrwxrwx    1 root     root          14 Dec  1 07:02 tls.key -> ..data/tls.key
lrwxrwxrwx    1 root     root          21 Dec  1 07:02 truststore.p12 -> ..data/truststore.p12

You have configured for /opt/jetty-home-11.0.13/frontend/etc/ssl/keystore.p12
Which is a symlink to a file reference at /opt/jetty-home-11.0.13/frontend/etc/ssl/..data/keystore.p12
But the /..data/ directory is symlinked to /opt/jetty-home-11.0.13/frontend/etc/ssl/..2022_12_01_07_19_44.534825794/
Resulting in a resolved symlink to the final file at /opt/jetty-home-11.0.13/frontend/etc/ssl/..2022_12_01_07_19_44.534825794/keystore.p12

@chetan777in
Copy link
Author

Yes , you are right. This is how kubernetes mounts secrets inside pod.

Also, when certificates are updated directory ..2022_12_01_07_19_44.534825794 will be renamed to new name with current timestamp resulting in keystore.p12 symlink pointing to a new final target

@joakime joakime changed the title Does ssl-reload monitor keystore when it is a symlink ? in jetty 9.4.x ? Does ssl-reload monitor keystore when it is a symlink? Dec 6, 2022
joakime added a commit that referenced this issue Dec 6, 2022
…nges

+ Removed changes from #8786 and #8787
+ More test cases

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Dec 6, 2022
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Copy link
Contributor

joakime commented Dec 6, 2022

Opened PR #9014 to resolve this in a different way

joakime added a commit that referenced this issue Dec 7, 2022
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Dec 7, 2022
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Dec 7, 2022
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Dec 7, 2022
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Dec 7, 2022
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Dec 7, 2022
…nges (#9014)

* Issue #8973 - Rework KeyStoreScanner handling for symlink related changes

+ Removed changes from #8786 and #8787
+ More test cases
+ revert jetty.sslContext.reload.followLinks boolean
+ Scanner should follow its own linkOptions setting
+ remove bad documentation in module-ssl-reload.adoc

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Co-authored-by: Lachlan Roberts <lachlan@webtide.com>
@chetan777in
Copy link
Author

@joakime Will this fix be available in upcoming jetty 11.0.13 ? If not in which next jetty 11 release will it available and what is the expected timeline for it ?

@joakime
Copy link
Contributor

joakime commented Dec 8, 2022

@chetan777in you can test 11.0.13 right now (it's staged and being tested before official release ATM)

The staging (maven) repo is https://oss.sonatype.org/content/groups/jetty-with-staging/

Just an FYI: the jetty.sslContext.reload.followLinks=true configuration doesn't exist anymore.

@joakime
Copy link
Contributor

joakime commented Dec 8, 2022

@chetan777in
Copy link
Author

I tested the fix from staging repo and it is working as expected . Thanks!

@joakime
Copy link
Contributor

joakime commented Dec 9, 2022

@chetan777in 11.0.13 has been released.

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

Successfully merging a pull request may close this issue.

4 participants