light weight Msi auth helper library to be used by SQL#2755
light weight Msi auth helper library to be used by SQL#2755praries880 merged 14 commits intoAzure:masterfrom
Conversation
|
Can one of the admins verify this patch? |
...ar/src/main/java/com/microsoft/azure/msiAuthTokenProvider/MSIConfigurationForAppService.java
Outdated
Show resolved
Hide resolved
| private static String getTokenFromResult(String result) { | ||
| int startIndex_AT = result.indexOf(ActiveDirectoryAuthentication.ACCESS_TOKEN_IDENTIFIER) | ||
| + ActiveDirectoryAuthentication.ACCESS_TOKEN_IDENTIFIER.length(); | ||
|
|
There was a problem hiding this comment.
should we add a sanity check here?
If substring not found then startIndex_AT is -1 and below substring op with -1 will throw IndexOutOfBoundsException. We could return null instead, thoughts?
There was a problem hiding this comment.
The only time this is gonna happen is if the result does not hold a token in it. That would happen in a error scenario... which would fall into the exception category.
But I do like defensive programming...will add the check mate :)
There was a problem hiding this comment.
thanks :) yes if it happens then it's an error scenario. When MSAL take this over, we can ask there preference on handling this, this may never occur or if happens they might want to throw client side exception (like TokenRetrievalException).
| } else throw ioEx; | ||
| } catch (Exception e){ | ||
| if (e.getCause()!= null && e.getCause() instanceof SocketException && e.getCause().getMessage().contains("Permission denied: connect")) { | ||
| throw new FileNotFoundException("Managed identity not found/configured"); |
There was a problem hiding this comment.
Can we throw an Exception with the actual exception wrapped in it? something like our own
MSITokenRetrievalException with the same msg but inner exception as the actual exception? In case of getTokenForVirtualMachineFromIMDSEndpoint method we throw RuntimeException without loosing the actual exception when possible, but I guess the same specialized exception is better there as well, what do you think?
There was a problem hiding this comment.
ill sync up woith you offline about this...
| public Single<MSIToken> getToken(String tokenAudience) { | ||
| switch (hostType) { | ||
| case VIRTUAL_MACHINE: | ||
| return Single.fromCallable(() -> this.getTokenForVirtualMachineFromIMDSEndpoint(tokenAudience == null ? this.configForVM.resource() : tokenAudience)); |
There was a problem hiding this comment.
Just a thought: We could make it async all the way down, i.e. by making getTokenForVirtualMachineFromIMDSEndpoint to use rx constructs that way retry-sleep is also async. May be we could keep this in mind and implement if there is a need.
There was a problem hiding this comment.
I am thinking of totally doing away with the rx dependency of this library... its meant to be a really light weight tar with a small footprint. The only reason I left this in was to keep it close to the original implementation here
There was a problem hiding this comment.
Completely agree that lib should have light weight footprint. At the same time I'm not sure about exposing only sync variant to retrieve the token. We have had customers asking for full async support including the authentication step, this is why we introduced AsyncServiceClientCredentials. We might want to think how to handle it.
We can always expose sync and let customer use rx/reactor construct to convert that to async. one problem I see is - customer may opt for io thread for this async conversion thinking token retrival is an nw io operation. But our retry strategy has back-off that can put this io thread in sleep mode, which is not recommend.
...oken-provider-jar/src/main/java/com/microsoft/azure/msiAuthTokenProvider/MSICredentials.java
Outdated
Show resolved
Hide resolved
3 participants
This PR creates a small jar file to be used by sql to pull tokens for managed identities.