Set the signing clock property #4341
Conversation
| /** | ||
| * Keeps common signing properties used by different signers. | ||
| */ |
There was a problem hiding this comment.
This makes sense. I was thinking about this a bit when I was working on some of the v4+v4a attributes, but decided we probably shouldn't do it for those attributes. It makes a lot of sense for the signing clock, though.
I think we should be really strict in the javadoc about this. It should be things that CAN NEVER be signer-specific. A clock is universal, it makes sense here. We need to write the javadoc in a way that keeps future developers from wanting to push something like service signing name here. In my opinion, clock is probably the only thing that ever makes sense here, but who knows what the future will hold...
There was a problem hiding this comment.
My initial thought was this goes in the HttpSigner interface itself and specific signers javadoc whether they use this property or not. What do folks think about that?
core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSignerCommon.java
Outdated
Show resolved
Hide resolved
core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/SharedSignerProperty.java
Outdated
Show resolved
Hide resolved
| // Assert that the time offset set was zero | ||
| assertThat(signRequest.property(SharedSignerProperty.SIGNING_CLOCK)).isNotNull(); |
There was a problem hiding this comment.
I think rather not set the property if offset is 0
There was a problem hiding this comment.
That will introduce a branch that is not needed the way it's written now, so I think is better just to have a single code path. Can you please help me understand why you think is better not to set it?
There was a problem hiding this comment.
I thought it is better since with current approach the test is more fragile and can fail sometimes if for some reason Instant.now is actually beyond the within value?
There was a problem hiding this comment.
The other test is still fragile like that. I think we can have a package-private constructor that takes in a Clock (instead of Clock.systemUTC), so that unit test can use a mock Clock and test exactly, instead of isCloseTo.
There was a problem hiding this comment.
That would address my main concern - test fragility. We could still chose to not set or set the clock property if offset is 0. I was thinking that if there is no need for offset, the SigningStage can let the HttpSigner decide whether it needs a clock at all or not. But I don't feel strongly about that, if the test fragility is addressed.
There was a problem hiding this comment.
I'd rather tackle the problem when and if arises, I'm know that what you describing is possible but I don't think is likely, for that to happen it would mean that the machine running the tests will need to park the thread for more than 100ms (~ 300 Million instructions). Were the previous tests brittle like actually failing every so often?
core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSigner.java
Outdated
Show resolved
Hide resolved
|
SonarCloud Quality Gate failed.
|
| @@ -67,7 +70,7 @@ | |||
|
|
|||
| @RunWith(MockitoJUnitRunner.class) | |||
| public class AsyncSigningStageTest { | |||
There was a problem hiding this comment.
Can we add a test (sync and async both) to assert that if the selected auth scheme has a SIGNING_CLOCK it is used over the default SIGNING_CLOCK? LGTM otherwise.
There was a problem hiding this comment.
good call about that use case!
Motivation and Context
The new signer interface needs to be aware of any detected time offset to use that when signing. We cannot set that property properly since we don't have that information inside the auth interceptor, so unlike the other properties, we set this when we create the request. The way it's currently implemented follows the same logic that the previous signers were using before.
Modifications
Modified the sync/async signing stages to grab from the dependencies the current timeoffset and use it to construct a
Clockthat the signer can use to compute the singing date.Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License