grpc / grpc-java

The Java gRPC implementation. HTTP/2 based RPC
https://grpc.io/docs/languages/java/
Apache License 2.0
11.45k stars 3.85k forks source link

AdvancedTlsX509TrustManager throws confusing exception if certificate file is missing #10221

Open Hodkinson opened 1 year ago

Hodkinson commented 1 year ago

What version of gRPC-Java are you using?

1.51

What is your environment?

Linux, Mac, JDK 17

What did you expect to see?

A file not found exception.

What did you see instead?

java.security.GeneralSecurityException: Files were unmodified before their initial update. Probably a bug.
       at io.grpc.util.AdvancedTlsX509TrustManager.updateTrustCredentialsFromFile(AdvancedTlsX509TrustManager.java:226) ~[grpc-core-1.51.0.jar:1.51.0]

Steps to reproduce the bug

make this call with a file created where there is no path:

AdvancedTlsX509TrustManager.newBuilder()
        .build()
        .also {
            it.updateTrustCredentialsFromFile(File(noFileAtPath), 1, TimeUnit.HOURS, executor)
        }

The updatedTime comes back as 0 from the readAndUpdate call.

erm-g commented 1 year ago

Hi @Hodkinson, thank you for the reporting! So I see the same exception after executing AdvancedTlsX509TrustManager.newBuilder().build(). updateTrustCredentialsFromFile(new File("I_don't_exist"), 1, TimeUnit.SECONDS, Executors.newSingleThreadScheduledExecutor()); What do you mean by

The updatedTime comes back as 0 from the readAndUpdate call. ?

Hodkinson commented 1 year ago

At line 224 of AdvancedTlsX509TrustManager: https://github.com/grpc/grpc-java/blob/master/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java#L224

there's a call to readAndUpdate, which assumes the file exists, but because it doesn't, a zero is returned, as lastModified returns 0, and the oldTime is also zero. This readAndUpdate seems a little fragile due to this assumption - for example a certificate could be removed on disk, rather than replaced. In this case, the last modified would be zero, which would not match the oldTime, and then it would try to construct the FileInputStream from a missing file.

One solution would be to assume in readAndUpdate that if the lastModified call returns zero, then the file does not exist, and throw a GeneralSecurityException reporting that the file is missing.

Hodkinson commented 1 month ago

Hi, any thoughts on this, do you need any extra information?