Closed epag closed 2 months ago
Just remembered I still need to look for any references to "changeit" in the unit tests. Doing that in a bit,
Hank
Here is the instance of "changeit" I found in the unit tests:
wres-tasker/test/wres/tasker/WresJobTest.java: char[] trustStorePassphrase = "changeit".toCharArray();
The reference to "changeit" is used in the setup method that is essentially creating a pseudo-tasker instance to test against. In addition to "changeit", there is a hardcoded reference to "wres-tasker-passphrase". I don't see any of this as being a problem in a unit test environment. However, if we use "wres-tasker-passprhase" hardcoded elsewhere, that could be an issue.
So, I grepped for "passphrase" and found noting matching "wres-tasker-passphrase", so that is likely unique to the unit test. Good. However, I did find two potential problematic references:
wres-tasker/src/wres/tasker/Tasker.java: sslContextFactory.setKeyStorePassword( "wres-web-passphrase" );
wres-messages/src/main/java/wres/messages/BrokerHelper.java: + "-passphrase" ).toCharArray();
For the tasker, the hardcoded pass phrase appears to be assumed for the cert provided via system property, "wres.taskerPathToServerP12
. If that system property is not specified, it defaults to wres-tasker_server_private_key_and_x509_cert.p12
. So, essentially the tasker is coded to assume that the password for any provided server P12 is wres-web-passphrase
. Do we want to continue to have a default .p12 file name if one is not provided? Or should the path be a required system property?
As for the BrokerHelper
reference, that is also tied to the tasker. Specifically:
public static SSLContext getSSLContextWithClientCertificate( Role role )
{
String ourClientCertificateFilename = BrokerHelper.getSecretsDir() + "/"
+ "wres-" + role.name()
.toLowerCase()
+ "_client_private_key_and_x509_cert.p12";
char[] keyPassphrase = ("wres-" + role.name().toLowerCase()
+ "-passphrase").toCharArray();
...
That method is called by the tasker as part of WresJob.initializeBrokerConnectionFactory
and as part of the static block in BrokerManagerHelper
.
Bottom line, the tasker is still using hardcoded information to connect to the broker. Let me see if I can piggy back off of the system properties and process already being used in BrokerHelper for the worker connect.
Hank
I guess the same question actually applies to both the tasker server .p12 and the truststore needed to communicate with the broker:
Do we want to require the properties be specified indicating the path of the pertinent file and the password, or do we want to have default values available if not specified?
Thoughts?
Hank
I think that if no password is provided it is assumed it is a null password (Since that is an option you can do). We shouldn't have any hard coded passwords in the codebase though
If you set defaults, make sure you log a clear warning that they are being used (edit: and the properties to override for something different). Also, don't fall back on the defaults if a path/password is declared and fails.
So Evan says go without passwords, period, while James response applies if we do have a hardcoded default password. So a 'no' plus a 'but if you do do it' equals a 'no' in my mind. I guess that means avoid passwords in the code base, period. Without a default password, it makes little sense to have a default path, right? So just require system properties be available that point to the .p12 and password, and, if not found, except out.
I'm still looking through the code to understand the implications of what I'm finding in the tasker/broker communications. Focusing on the BrokerHelper
code, it appears as though the broker is saying, if you want to talk to me, I need to authenticate you, and you need to provide me a file with a specific name format, wres-[role]_client_private_key_and_x509_cert.p12
and password `wres-[role]-passphrase'.
There are two things I need to look into:
I can see the wres-worker_client_private_key_and_x509_cert.p12
file referenced in the cert spreadsheet. How come I can't find any calls to BrokerHelper.getSSLContextWithClientCertificate
from the worker side? It need to talk to the broker, as well, so how does it do that?
The .p12 with passphrase is being handed off by the tasker to the broker when setting up an SSL context. Presumably, that means the broker knows how to authenticate that .p12 with a CA .pem on its end. Is that the purpose of the truststore for which I added environment variables yesterday? I assume so, but need to confirm.
I guess question one is the biggest one for me, since I think I have the answer to question 2. I need to step away for a few minutes, but will look into it further when I return.
Hank
To clarify, a null password can be the default. A default path and a default (null) password is fine, but it certainly makes no sense to have a default non-null password, even though changeit
is the standard in this context.
Got it. Thanks!
When I get to coding the default, I'll have it look at the property and, if not found or empty, use null as the password, which would imply that the trust store has no password set, I guess.
As for the path to the file, do we want to have a default path built if not provided by property, or do we want to require the path be fully specified.
Back in a bit,
Hank
- I can see the
wres-worker_client_private_key_and_x509_cert.p12
file referenced in the cert spreadsheet. How come I can't find any calls toBrokerHelper.getSSLContextWithClientCertificate
from the worker side? It need to talk to the broker, as well, so how does it do that?
Worker::createConnectionFactory
And, yes, it appears a lot of stuff is hardcoded/assumed by getSSLContextWithClientCertificate
, including the path, password and even the protocol version. I suppose they should all be supplied as inputs.
A minor thing. I am pretty sure that paths not to the wres-tasker_client_private_key_and_x509_cert.p12 (the client cert for the tasker)
The wres-web-passphrase
cert refers to the tasker server cert. So its the dev_server cert one instead. I think that is why we only reference that as we do not have a worker server cert created to my knowledge
Its a pain since we just re-did the certs, but we should create new passwords/update any certs that were present in the codebase
The tasker is an http server and an amqp client/peer. The worker is an amqp client/peer (not an http server).
James:
Thanks for pointing me to that. It turns out that Eclipse was not finding the call to getSSLContextWithClientCertificate
because of the build problems on my laptop I mentioned before. For some reason, it can't resolve BrokerHelper within that class. I'm hoping that's the only call I'm missing (I think so), but I'm going to do a brute force string search to be sure.
I don't plan to address the protocol version at this point. Maybe in the future in another ticket.
So basically getSSLContextWithClientCertificate
needs to allow for file name and password to be provided. The filename will be required to be non-null. I don't see any clear response above to my question about whether having a default path makes sense. I think I'd rather just require the property, so I'll go with my feelings on that one.
Both the tasker and workers, then, will need to include a path property that can point to the .p12. Note that the tasker already includes a property to point to its own HTTP server P12: wres.taskerPathToServerP12
. I think the new property should be, wres.taskerPathToBrokerP12
and wres.workerPathToBrokerP12
. However, I'm a bit worried the name could be confusing; that is, "Why does the tasker and worker need a broker P12?". Can either of you think of a clearer yet succinct way to name that property? Keep in mind this is the .p12 file that the broker will see in order to authenticate the tasker/worker.
The password property that corresponds to the above new properties will be named similarly. Once I have the path property names, hopefully a password property name will become obvious.
Thanks,
Hank
I wouldn't put broker in the name, this is merely a worker or tasker p12 bundle to be supplied (to the broker). I would name wres.taskerPathToP12Bundle
etc. Note that there is no client/server architecture with brokered communication to begin with. It isn't that the broker is a server, for example. The tasker is an http server too, though.
If you want to avoid confusion/overlap with the http server, you can qualify the worker and tasker as amqp clients and hence wres.taskerPathToClientP12Bundle
, if you prefer. But, again, there is no server in this context, the broker isn't one.
James: Thanks. I'll use that second one:
wres.taskerPathToClientP12Bundle
wres.workerPathToClientP12Bundle
The password would just slap Password
on the end. And I did confirm that the tasker and workers are the only places where the method in question gets called.
All:
Here are the changes... BrokerHelper.getSSLContextWithClientCertificate
in question will be modified to ask for a path and a password instead of 'Role'. If the path is not supplied, I'm going to have that method throw an IllegalArgumentException
indicating that the required path was null or empty. However, I'll probably have the callers do an upfront check to confirm that the path property is set and throw an IllegalStateException
if not (or other more appropriate exception; I'll scan the code for examples to base it on) so that a more understandable exception message can be output.
As for the password, as said above, if the property isn't found, no password is assumed. A log message of some sort will be output. Since we password protect both files, we'll need to include it in the java options for the tasker and worker.
The mapped environment variables will have the same names as the properties but all caps with underscores.
Sound reasonable?
Hank
Sounds fine. If the Role
isn't used elsewhere, following this change, please delete it.
Got it. I'll check out Role to see if its used elsewhere.
For the worker, the new system properties are going to go into the JAVA_OPTS
within the compose, not the INNER_JAVA_OPTS
, since the changes are going impact the worker-shim talking to the broker, not the worker, itself. If that's wrong, let me know.
Hank
Sounds accurate.
I have the code and compose changes in. I'm going to do some testing through the -dev COWRES.
Hank
The -dev COWRES appears to be operational. Yay! However, I still need to test sad-path scenarios where environment variables are missing or passwords incorrectly identified. I'll do that this afternoon.
I have the code committed locally to facilitate testing a full deployment where the compose entry file is fully built.
Note that, with all of the changes, our -dev COWRES .env now looks like what I've shared below with some information (probably more than needed) omitted. Its important to realize that all of the truststore and keystore passwords are being exposed in the .env
file. If that poses a security risk for any of the files involved, we'll need to address that. It shouldn't, since the passwords were hardcoded into the code base before.
Its also worth noting that the environment variables need to be changed to reflect what Evan has done for the NWC environment.
Thanks,
Hank
=============================
DOCKER_REGISTRY=[OMITTED]
WRES_ENV_SUFFIX=-dev
WRES_DB_FQDN=[OMITTED]
NFS_HOME_DIR_DEVICE=[omitted]:/home
NFS_HOME_DIR_IP_ADDRESS=[omitted]
NFS_WRES_SHARE_DIR_IP_ADDRESS=[omitted]
NFS_WRES_SHARE_DIR_DEVICE=[omitted]:/wresdocker
WRES_BROKER_HOST=[omitted]
# Accessing the Postgres database. No password used.
WRES_DB_CA_FILE=/wres_secrets/DODSWCA_60.pem
# Accessing WRDS. No password used.
WRDS_CA_FILE=/wres_secrets/DODSWCA_60.pem
# General trust store for all of the WRES.
WRES_TRUST_STORE=/wres_secrets/trustedCertificateAuthorities.jks
WRES_TRUST_STORE_PASSWORD=[omitted]
# DStore access options.
DSTORE_TRUST_STORE=/wres_secrets/dstoreTrustStore.jks
DSTORE_TRUST_STORE_PASS=[omitted]
#NGINX cert options.
NGINX_SERVER_CERT=/mnt/wres_keys/metrics_x509_cert.pem
NGINX_SERVER_KEY=/mnt/wres_keys/metrics_private_rsa_key.pem
# Tasker and worker cert options and passwords.
WRES_ADMIN_TOKEN=[omitted]
WRES_TASKER_SERVER_P12=/wres_secrets/nwcal-wres-dev_server_private_key_and_x509_cert.p12
WRES_TASKER_SERVER_P12_PASSWORD=[omitted]
WRES_TASKER_CLIENT_P12=/wres_secrets/wres-tasker_client_private_key_and_x509_cert.p12
WRES_TASKER_CLIENT_P12_PASSWORD=[omitted]
WRES_WORKER_CLIENT_P12=/wres_secrets/wres-worker_client_private_key_and_x509_cert.p12
WRES_WORKER_CLIENT_P12_PASSWORD=[omitted]
WRES_MONITOR_PASSWORD=[omitted]
# Broker cert options. Note that all names are from *inside the container*.
WRES_RABBITMQ_SSL_OPTIONS_CACERTFILE=/wres_secrets/ca_x509_cert.pem
WRES_RABBITMQ_SSL_OPTIONS_CERTFILE=/wres_secrets/broker_server_x509_cert.pem
WRES_RABBITMQ_SSL_OPTIONS_KEYFILE=/wres_secrets/broker_server_private_rsa_key.pem
WRES_RABBITMQ_MANAGEMENT_SSL_CACERTFILE=/wres_secrets/ca_x509_cert.pem
WRES_RABBITMQ_MANAGEMENT_SSL_CERTFILE=/wres_secrets/broker_server_x509_cert.pem
WRES_RABBITMQ_MANAGEMENT_SSL_KEYFILE=/wres_secrets/broker_server_private_rsa_key.pem
WRES_RABBITMQ_PROMETHEUS_SSL_CACERTFILE=/wres_secrets/OWPAL-CA-60.pem
WRES_RABBITMQ_PROMETHEUS_SSL_CERTFILE=/wres_secrets/metrics_x509_cert.pem
WRES_RABBITMQ_PROMETHEUS_SSL_KEYFILE=/wres_secrets/metrics_private_rsa_key.pem
# Events broker cert options. Note that all names are fron *inside the container*.
# Also note that the passphrases are not important to secure.
EVENTSBROKER_KEYSTORE_PATH=/wres_secrets/wres-eventsbroker_server_keystore.p12
EVENTSBROKER_KEYSTORE_PASSWORD=[omitted]
EVENTSBROKER_TRUSTSTORE_PATH=/wres_secrets/wres-eventsbroker_server_truststore.p12
EVENTSBROKER_TRUSTSTORE_PASSWORD=[omitted]
So I ran a couple more evaluations, including one using WRDS, and they all succeeded.
However, when I brought down the service, commented out the tasker and worker password env variables, and brought it back up, I got NPEs:
java.lang.IllegalStateException: WRES expected to be able to read and decrypt the file '/wres_secrets/wres-tasker_client_private_key_and_x509_cert.p12'.
at wres.messages.BrokerHelper.getSSLContextWithClientCertificate(BrokerHelper.java:332)
at wres.tasker.WresJob.initializeBrokerConnectionFactory(WresJob.java:1014)
at wres.tasker.WresJob.<clinit>(WresJob.java:144)
at wres.tasker.Tasker.main(Tasker.java:138)
Caused by: java.security.UnrecoverableKeyException: Get Key failed: Cannot read the array length because "password" is null
at java.base/sun.security.pkcs12.PKCS12KeyStore.engineGetKey(PKCS12KeyStore.java:450)
at java.base/sun.security.util.KeyStoreDelegator.engineGetKey(KeyStoreDelegator.java:91)
at java.base/java.security.KeyStore.getKey(KeyStore.java:1050)
at java.base/sun.security.ssl.SunX509KeyManagerImpl.<init>(SunX509KeyManagerImpl.java:141)
at java.base/sun.security.ssl.KeyManagerFactoryImpl$SunX509.engineInit(KeyManagerFactoryImpl.java:64)
at java.base/javax.net.ssl.KeyManagerFactory.init(KeyManagerFactory.java:275)
at wres.messages.BrokerHelper.getSSLContextWithClientCertificate(BrokerHelper.java:328)
... 3 common frames omitted
Caused by: java.lang.NullPointerException: Cannot read the array length because "password" is null
at java.base/sun.security.pkcs12.PKCS12KeyStore$RetryWithZero.run(PKCS12KeyStore.java:259)
at java.base/sun.security.pkcs12.PKCS12KeyStore.engineGetKey(PKCS12KeyStore.java:361)
... 9 common frames omitted
Exception in thread "main" java.lang.ExceptionInInitializerError
at wres.tasker.Tasker.main(Tasker.java:138)
Caused by: java.lang.IllegalStateException: WRES expected to be able to read and decrypt the file '/wres_secrets/wres-tasker_client_private_key_and_x509_cert.p12'.
at wres.messages.BrokerHelper.getSSLContextWithClientCertificate(BrokerHelper.java:332)
at wres.tasker.WresJob.initializeBrokerConnectionFactory(WresJob.java:1014)
at wres.tasker.WresJob.<clinit>(WresJob.java:144)
... 1 more
Caused by: java.security.UnrecoverableKeyException: Get Key failed: Cannot read the array length because "password" is null
at java.base/sun.security.pkcs12.PKCS12KeyStore.engineGetKey(PKCS12KeyStore.java:450)
at java.base/sun.security.util.KeyStoreDelegator.engineGetKey(KeyStoreDelegator.java:91)
at java.base/java.security.KeyStore.getKey(KeyStore.java:1050)
at java.base/sun.security.ssl.SunX509KeyManagerImpl.<init>(SunX509KeyManagerImpl.java:141)
at java.base/sun.security.ssl.KeyManagerFactoryImpl$SunX509.engineInit(KeyManagerFactoryImpl.java:64)
at java.base/javax.net.ssl.KeyManagerFactory.init(KeyManagerFactory.java:275)
at wres.messages.BrokerHelper.getSSLContextWithClientCertificate(BrokerHelper.java:328)
... 3 more
Caused by: java.lang.NullPointerException: Cannot read the array length because "password" is null
at java.base/sun.security.pkcs12.PKCS12KeyStore$RetryWithZero.run(PKCS12KeyStore.java:259)
at java.base/sun.security.pkcs12.PKCS12KeyStore.engineGetKey(PKCS12KeyStore.java:361)
... 9 more
The reason is because, keyManagerFactory.init( keyStore, keyPassphrase );
, cannot handle a null passphrase.
Will look into that after a meeting,
Hank
Probably needs an empty character array.
Right. I'm just going to look up the code Javadoc to confirm,
Hank
The Javadoc doesn't talk about null or empty arrays, so it wasn't helpful. I made the change to use an empty array if no password is provided and testing it results in the expected log message in the tasker:
Exception in thread "main" java.lang.ExceptionInInitializerError
at wres.tasker.Tasker.main(Tasker.java:138)
Caused by: java.lang.IllegalStateException: WRES expected to find a file '/wres_secrets/wres-tasker_client_private_key_and_x509_cert.p12' with PKCS#12 format, with both a client certificate AND the private key inside, used to authenticate to the broker.
at wres.messages.BrokerHelper.getSSLContextWithClientCertificate(BrokerHelper.java:305)
at wres.tasker.WresJob.initializeBrokerConnectionFactory(WresJob.java:1014)
at wres.tasker.WresJob.<clinit>(WresJob.java:144)
... 1 more
Caused by: java.io.IOException: keystore password was incorrect
at java.base/sun.security.pkcs12.PKCS12KeyStore.engineLoad(PKCS12KeyStore.java:2159)
at java.base/sun.security.util.KeyStoreDelegator.engineLoad(KeyStoreDelegator.java:221)
at java.base/java.security.KeyStore.load(KeyStore.java:1473)
at wres.messages.BrokerHelper.getSSLContextWithClientCertificate(BrokerHelper.java:301)
... 3 more
and in the workers:
Exception in thread "main" java.lang.IllegalStateException: WRES expected to find a file '/wres_secrets/wres-worker_client_private_key_and_x509_cert.p12' with PKCS#12 format, with both a client certificate AND the private key inside, used to authenticate to the broker.
at wres.messages.BrokerHelper.getSSLContextWithClientCertificate(BrokerHelper.java:305)
at wres.worker.Worker.createConnectionFactory(Worker.java:205)
at wres.worker.Worker.main(Worker.java:105)
Caused by: java.io.IOException: keystore password was incorrect
at java.base/sun.security.pkcs12.PKCS12KeyStore.engineLoad(PKCS12KeyStore.java:2159)
at java.base/sun.security.util.KeyStoreDelegator.engineLoad(KeyStoreDelegator.java:221)
at java.base/java.security.KeyStore.load(KeyStore.java:1473)
at wres.messages.BrokerHelper.getSSLContextWithClientCertificate(BrokerHelper.java:301)
... 2 more
Cool. I'm going to bring back the environment variables. cycle the -dev COWRES, and then focus on my 3pm meeting. That's it for me today for this ticket. I'll complete testing tomorrow and get the code pushed.
Hank
A couple of oddities this morning. First, I can't visit any of the broker monitors. I'm assuming this may be related to cert changes made by Evan. However, I don't recall seeing the problem yesterday when I visited the broker monitor (at least I think I looked at it yesterday).
Second, the workers on the worker-only machine, -dev02, cannot find the broker:
2024-09-06T11:58:33.310+0000 ERROR Worker Checked exception while talking to the broker.
Exception in thread "main" java.net.UnknownHostException: broker: Name or service not known
at java.base/java.net.Inet6AddressImpl.lookupAllHostAddr(Native Method)
at java.base/java.net.InetAddress$PlatformNameService.lookupAllHostAddr(InetAddress.java:934)
at java.base/java.net.InetAddress.getAddressesFromNameService(InetAddress.java:1543)
at java.base/java.net.InetAddress$NameServiceAddresses.get(InetAddress.java:852)
at java.base/java.net.InetAddress.getAllByName0(InetAddress.java:1533)
at java.base/java.net.InetAddress.getAllByName(InetAddress.java:1385)
at java.base/java.net.InetAddress.getAllByName(InetAddress.java:1306)
at com.rabbitmq.client.DnsRecordIpAddressResolver.resolveIpAddresses(DnsRecordIpAddressResolver.java:83)
at com.rabbitmq.client.DnsRecordIpAddressResolver.getAddresses(DnsRecordIpAddressResolver.java:73)
at com.rabbitmq.client.impl.recovery.RecoveryAwareAMQConnectionFactory.newConnection(RecoveryAwareAMQConnectionFactory.java:64)
at com.rabbitmq.client.impl.recovery.AutorecoveringConnection.init(AutorecoveringConnection.java:165)
at com.rabbitmq.client.ConnectionFactory.newConnection(ConnectionFactory.java:1242)
at com.rabbitmq.client.ConnectionFactory.newConnection(ConnectionFactory.java:1198)
at com.rabbitmq.client.ConnectionFactory.newConnection(ConnectionFactory.java:1156)
at com.rabbitmq.client.ConnectionFactory.newConnection(ConnectionFactory.java:1321)
at wres.worker.Worker.main(Worker.java:108)
I'm not sure what's causing that,
Hank
The name of the broker host comes from this setting, which has not changed in a long time:
-Dwres.broker=broker
That's where the "broker" comes from in, "broker: Name or service not known". I assume Docker magic is supposed to fill to map "broker" to the broker that resides on the other machine.
I may have to start using older compose YAMLs in order to see if there is something specific I did that broke that magic.
Hank
It's nothing more than the docker service name, which is broker
and, yes, it's on our configured docker network, wres_net
.
I think I see the mistake. I copied the worker-shim java options from the entry yml to the worker yml. A direct copy was not a good idea, because there are other differences such as, '-Dwres.broker=${WRES_BROKER_HOST}'. I need to back out some of my changes.
Hank
With that fix, its now the SSL handshake that is failing:
worker_1 | 2024-09-06T12:24:58.028+0000 ERROR Worker Checked exception while talking to the broker.
worker_1 | Exception in thread "main" javax.net.ssl.SSLHandshakeException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
The handshake for the workers on the entry machine succeeds; the handshake for those on the workers-only machine fails. Investigating,
Hank
Probably cannot find an expected ca cert.
I think it might be that the .p12 file (or files?) on the nwcal-wres-dev02 were not updated. Here is what I see on the workers-only, -dev02 machine:
[hank.herr@nwcal-wres-dev02 deployment]$ ls -l /mnt/wres_keys/wres-worker_client_private_key_and_x509_cert.p12
-rw-rw-r--. 1 hank.herr wres 3292 Jun 5 17:53 /mnt/wres_keys/wres-worker_client_private_key_and_x509_cert.p12
Here is what I see on the entry, -dev03 machine:
[hank.herr@nwcal-wres-dev03 deployment]$ ls -l /mnt/wres_keys/wres-worker_client_private_key_and_x509_cert.p12
-rwxr-xr-x. 1 evan.pagryzinsk wres 3125 Aug 14 17:50 /mnt/wres_keys/wres-worker_client_private_key_and_x509_cert.p12
Evan: Can I just copy the cert files from the -dev03 to the -dev02?
I need to step away for about 15 minutes,
Hank
Yeah, you should be good to copy whatever you need. I actually cannot remember if I coppied over the dev certs from dev03 to dev02. Let me check
Ah yeah, looks like I just forgot to update the certs on dev02. Ill do that in a minute
Certs were updated, cycled the containers, and they look good now
Thanks, Evan!
I was able to bring up the workers machine in -dev and run 3 evaluations at once, proving that at least one worker on the workers machine was up and running. Sweet.
I'm in a meeting for the next hour, but plan to do the pull request when I'm done.
Note that I still don't have access to any broker or events broker monitors. I'm guessing that's due to the cert changes. Evan: I'll try to work with you later today to get my certs updated so that I can access those monitors.
Also note that I need to rerun the JUnit tests on Linux to see if they still freeze. If so, I'll create a ticket and try to resolve that next week. I think if you scroll way up in these comments, you'll see what I'm talking about.
But, again, meeting now, then pull request, then the rest.
Hank
The wiki has been updated on how to create the new monitoring certs, let me know if you have any questions
Cool. Thanks.
I'm still unable to run the unit tests for the same reason. I'll create a ticket for that later.
I did modify a unit test under wres-tasker, iirc. I'm going to see if there is a way to run just that set of unit tests.
Hank
Just as a reminder, the unit tests will automatically run when you create a pr/commit code if you cant get them working on the gov box
Understood. Will that prevent me from breaking the master branch? If so, I'll just let the pull request tell me if the unit test is broken. If not, I'd rather address it locally first.
Hank
Yeah, things will only make it to master once you merge your PR. You could override all of the safety checks, but it should be exceptionally hard to break master accidentally
It was BrokerHelperTest
that was modified, and I'm not sure if other unchanged unit tests are impacted.
I'll do the PR and see what happens,
Hank
The only thing to remember is to allow all of the automated checks to complete before you override branch controls and merge the PR, at which point you are only overriding the requirement for a code review.
I just remembered that you dont even need a PR yet. Just pushing the commits on your branch kicks off the tests https://github.com/NOAA-OWP/wres/commits/github_296
https://github.com/NOAA-OWP/wres/actions/runs/10740135494/job/29787508254
Hank, just a heads up that "refs" isn't a github keyword w/r to connecting git commit messages to tickets, but it is sufficient to supply the hash and the ticket number, as you did.
I've marked this UAT as a reminder that we need to account for .env changes when we deploy 6.26 to the NWC. There is no testing to do beyond setting the .env and making sure everything still works.
Hank
James: Right. I figured that out once I saw the pull request. It was able to link the tickets just based on a '#'. Evan also sent me a link with key words that are valid.
Evan: Thanks. I'll remember that in the future: only the push is needed to trigger unit tests.
Appreciate your help fighting through this! I'll look into the monitoring certs after lunch and then start investigating the unit test issues in the other ticket.
Hank
Unit tests are failing for me:
Caused by: java.lang.IllegalStateException: The system property wres.taskerPathToClientP12Bundle is not specified. It must be provided and non-empty.
at wres.tasker.WresJob.initializeBrokerConnectionFactory(WresJob.java:1000)
at wres.tasker.WresJob.<clinit>(WresJob.java:144)
... 59 more
These unit tests shouldn't depend on the presence of system properties supplied at runtime.
He currently have hard coded cert names in our broker
https://github.com/NOAA-OWP/wres/blob/bc993975a9ce41be6d0dc9eb54daa36d8ecb4e1c/wres-broker/nonsrc/rabbitmq.conf#L10-L23
Also there is this hard coded pass for our trust store. Which doesn't really do anything but for craftmanship should be extracted as well ( think it also shows up in a test file as well)
https://github.com/NOAA-OWP/wres/blob/bc993975a9ce41be6d0dc9eb54daa36d8ecb4e1c/wres-system/src/wres/system/SSLStuffThatTrustsOneCertificate.java#L111-L115
https://github.com/NOAA-OWP/wres/blob/bc993975a9ce41be6d0dc9eb54daa36d8ecb4e1c/wres-messages/src/main/java/wres/messages/BrokerHelper.java#L171-L172