Azure-sdk-for-java: [BUG] KeyClient.listPropertiesOfKeyVersions(String) throws a HttpRequestException instead of ResourceNotFoundException if the key does not exist

[blacelle]

Describe the bug
KeyClient.listPropertiesOfKeyVersions javadoc states it will return a ResourceNotFoundException if the key does not exist. However, it returns a HttpRequestException with 404 errorCode

Exception or Stack Trace
Status code 404, "{"error":{"code":"KeyNotFound","message":"A key with (name/id) versions was not found in this key vault. If you recently deleted this key you may be able to recover it using the correct recovery command. For help resolving this issue, please see https://go.microsoft.com/fwlink/?linkid=2125182"}}"
com.azure.core.exception.HttpResponseException: Status code 404, "{"error":{"code":"KeyNotFound","message":"A key with (name/id) versions was not found in this key vault. If you recently deleted this key you may be able to recover it using the correct recovery command. For help resolving this issue, please see https://go.microsoft.com/fwlink/?linkid=2125182"}}"
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
at com.azure.core.http.rest.RestProxy.instantiateUnexpectedException(RestProxy.java:357)
at com.azure.core.http.rest.RestProxy.lambda$ensureExpectedStatus$3(RestProxy.java:398)
at reactor.core.publisher.MonoFlatMap$FlatMapMain.onNext(MonoFlatMap.java:118)
at reactor.core.publisher.Operators$MonoSubscriber.complete(Operators.java:1712)
at reactor.core.publisher.MonoCacheTime$CoordinatorSubscriber.signalCached(MonoCacheTime.java:320)
at reactor.core.publisher.MonoCacheTime$CoordinatorSubscriber.onNext(MonoCacheTime.java:337)
at reactor.core.publisher.Operators$ScalarSubscription.request(Operators.java:2274)
at reactor.core.publisher.MonoCacheTime$CoordinatorSubscriber.onSubscribe(MonoCacheTime.java:276)
at reactor.core.publisher.FluxFlatMap.trySubscribeScalarMap(FluxFlatMap.java:191)
at reactor.core.publisher.MonoFlatMap.subscribeOrReturn(MonoFlatMap.java:53)
at reactor.core.publisher.InternalMonoOperator.subscribe(InternalMonoOperator.java:48)
at reactor.core.publisher.MonoDefer.subscribe(MonoDefer.java:52)
at reactor.core.publisher.MonoCacheTime.subscribeOrReturn(MonoCacheTime.java:132)
at reactor.core.publisher.InternalMonoOperator.subscribe(InternalMonoOperator.java:48)
at reactor.core.publisher.MonoFlatMap$FlatMapMain.onNext(MonoFlatMap.java:150)
at reactor.core.publisher.FluxDoFinally$DoFinallySubscriber.onNext(FluxDoFinally.java:123)
at reactor.core.publisher.FluxHandle$HandleSubscriber.onNext(FluxHandle.java:112)
at reactor.core.publisher.FluxMap$MapConditionalSubscriber.onNext(FluxMap.java:213)
at reactor.core.publisher.FluxDoFinally$DoFinallySubscriber.onNext(FluxDoFinally.java:123)
at reactor.core.publisher.FluxHandleFuseable$HandleFuseableSubscriber.onNext(FluxHandleFuseable.java:178)
at reactor.core.publisher.FluxContextStart$ContextStartSubscriber.onNext(FluxContextStart.java:103)
at reactor.core.publisher.Operators$MonoSubscriber.complete(Operators.java:1712)
at reactor.core.publisher.MonoCollectList$MonoCollectListSubscriber.onComplete(MonoCollectList.java:121)
at reactor.core.publisher.FluxPeek$PeekSubscriber.onComplete(FluxPeek.java:252)
at reactor.core.publisher.FluxMap$MapSubscriber.onComplete(FluxMap.java:136)
at reactor.netty.channel.FluxReceive.terminateReceiver(FluxReceive.java:421)
at reactor.netty.channel.FluxReceive.drainReceiver(FluxReceive.java:211)
at reactor.netty.channel.FluxReceive.onInboundComplete(FluxReceive.java:369)
at reactor.netty.channel.ChannelOperations.onInboundComplete(ChannelOperations.java:367)
at reactor.netty.channel.ChannelOperations.terminate(ChannelOperations.java:416)
at reactor.netty.http.client.HttpClientOperations.onInboundNext(HttpClientOperations.java:612)
at reactor.netty.channel.ChannelOperationsHandler.channelRead(ChannelOperationsHandler.java:90)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:436)
at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:321)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:295)
at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:251)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1486)
at io.netty.handler.ssl.SslHandler.decodeNonJdkCompatible(SslHandler.java:1247)
at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1284)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:498)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:437)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:163)
at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:714)
at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:650)
at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:576)
at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:493)
at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989)
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.base/java.lang.Thread.run(Thread.java:834)

To Reproduce
Azure-security-keyVault-4.1.2

[joshfree]

@vcolin7 can you please investigate?

[vcolin7]

I did some digging and I could not reproduce getting an HttpResponseException while using the name of a key that does not exist and that is not an empty string.

It seems that, when using listPropertiesOfVersions for a key that doesn't exist, the KeyVault service returns a payload such as the following at the moment:{"value": [], "nextLink": null} and a status code 200. If we instead use an empty string as an argument for this method, the service answers with a status code 404 and a payload that looks like what was described by @blacelle and the library throws a HttoResponseException, which is expected.

This is a problem since we should be checking the response even in successful (200) cases to throw the appropriate exception (ResourceNotFoundException). We'll be adding this to our backlog.

I'm still curious about how you could get a 404 while providing a non-empty key name, would you happen to have a code snippet you can share where this happens?

[vcolin7]

Do you remember if the key name used was an empty or null string @blacelle? Please check my comment above where I outlined how I could not get a 404 unless I send an empty or null name, anything else returns 200.

[blacelle]

The key was definitely not null nor empty.

[vcolin7]

Closing this for the moment since this issue could not be reproduced. Please feel free to re-open it if you encounter this once more or if there is something I may have misunderstood.

[blacelle]

@vcolin7 Sorry for having lacked details in my report. As you did not reproduce, I looked again at my code. Here is a snippet describing what went wrong, and was possibly not a bug, but definitely confusive:

        KeyClient client = new KeyClientBuilder().vaultUrl("https://XXX.vault.azure.net/")
                .credential(new ClientSecretCredentialBuilder().clientId(clientId)
                        .clientSecret(clientSecret)
                        .tenantId(tenantId)
                        .build())
                .buildClient();

                // OK, do not throw
        client.listPropertiesOfKeyVersions("anotherUnknownKey");

                // OK ResourceNotFoundException
        client.getKey("anotherUnknownKey");

                // KO HttpResponseException, I feel this should return ResourceNotFoundException but you may disagree (it was confusive to me, as I first thought Azure lib was bugged, and later in the dev cycle, I spot the issue in my code (the '_' character))
        client.getKey("anotherUnknownKey_toto");

                // KO HttpResponseException, same remark as before. I got this when I mis-used kid, relying on kid="keyName/keyVersion", having chopped the VaultUrl manually.
        client.getKey("anotherUnknownKey/toto");

(For the record, my usage here is self-management of Key expiry).

[vcolin7]

Thanks for providing more details! It is always appreciated :)

As I mentioned earlier, the service will return a response with a status code 200 including an empty list of values for the name of a key that does not exist in a given Key Vault, which is why the client doesn't throw ResourceNotFoundException, which is mapped for 404 in numerous parts of the SDK.

Additionally, the reason getKey returns an HttpResponseException for a key name that includes underscores is because the service responds with a status code 400 and a message The request URI contains an invalid name: <key-name>.

On a similar note, if you provide a key name that contains a forward slash, the service will interpret you are trying to reach different endpoint than the one you originally intended, prompting a response that would not be mapped to a ResourceNotFoundException.

Finally, I do agree that receiving a status code 200 from the listVersions endpoint and getting 404 from getKey while using the same name for an unknown key might look inconsistent, although I don't have full knowledge behind the decision to do so.

[blacelle]

Why not checking the input key before sending the query ? It seems I may even consider a key starting with '../' to leak information, given the library does not check for valid characters.

Then an IllegalArgumentException seems very legitimate to me, and much less confusive than current behavior.

[vcolin7]

Those are valid points and I will be happy to bring them up with the team to see what we can add to our backlog for next releases. Thanks a lot for your feedback :)

[blacelle]

Hi @vcolin7, another case today: for some reason, we end being requested with a key wich is too long to be a valid key name. 'Method GET does not allow operation XXX' is very very expressive, while I guess we'd better checking for the input value in the client/before the API call.

2020-06-11T21:11:38.785334444Z Caused by: com.azure.core.exception.HttpResponseException: Status code 400, "{"error":{"code":"BadParameter","message":"Method GET does not allow operation 'verylongsecretname-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'"}}"
2020-06-11T21:11:38.785339644Z at jdk.internal.reflect.GeneratedConstructorAccessor49.newInstance(Unknown Source)
2020-06-11T21:11:38.785343544Z at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
2020-06-11T21:11:38.785347544Z at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
2020-06-11T21:11:38.785351344Z at com.azure.core.http.rest.RestProxy.instantiateUnexpectedException(RestProxy.java:357)
2020-06-11T21:11:38.785355244Z at com.azure.core.http.rest.RestProxy.lambda$ensureExpectedStatus$3(RestProxy.java:400)

[vcolin7]

Thanks for the additional input @blacelle :)