Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a fix to the error raised when using empty spiffeID in fetch_jwt_… #109

Merged

Conversation

telliere
Copy link
Contributor

@telliere telliere commented Mar 5, 2024

…svid

Modifying this part :

    @handle_error(error_cls=FetchJwtSvidError)
    def fetch_jwt_svid(
        self, audiences: List[str], subject: Optional[SpiffeId] = None
    ) -> JwtSvid:
        if not audiences:
            raise ArgumentError('Parameter audiences cannot be empty')
       
        response = self._spiffe_workload_api_stub.FetchJWTSVID(
            request=workload_pb2.JWTSVIDRequest(
                audience=audiences,
                spiffe_id=str(subject),
            )
        )

        if len(response.svids) == 0:
            raise FetchJwtSvidError('JWT SVID response is empty')

        svid = response.svids[0].svid
        return JwtSvid.parse_insecure(svid, audiences)

Bug :

When using fetch_jwt_svid with no spiffeID specified it raises a FetchJwtSvidError while it shouldn't.

Cause :

spiffe_id=str(subject),

parsing a None object as a str creates 'None', causing the next function (JWTSVIDRequest) to consider having a specified spiffeID when None has been specified, handled by the @handle_error as a FetchJwtSvidError.

Fix :

Handling the NoneType before, by casting subject to a str only if it's not None.

        if subject != None:
            subject = str(subject)

@maxlambrecht
Copy link
Collaborator

Thanks for this PR, @telliere!

Would you rebase your branch?

@telliere telliere force-pushed the feature/fix-empty-spiffeID branch from affadc0 to efa99d4 Compare March 5, 2024 16:17
@telliere
Copy link
Contributor Author

telliere commented Mar 5, 2024

Done,

Actually I have to say that the other PR opened would help me a lot, but I didn't have a look at the code

@telliere
Copy link
Contributor Author

telliere commented Mar 7, 2024

Was of the subject, sorry about that. Thanks for the review / The suggestions

Copy link
Collaborator

@maxlambrecht maxlambrecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @telliere!

@maxlambrecht maxlambrecht merged commit 7c02d76 into HewlettPackard:master Mar 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants