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

Extend aad app set to add certificate to already available App #3116

Closed
arjunumenon opened this issue Mar 8, 2022 · 12 comments
Closed

Extend aad app set to add certificate to already available App #3116

arjunumenon opened this issue Mar 8, 2022 · 12 comments
Assignees
Milestone

Comments

@arjunumenon
Copy link
Member

arjunumenon commented Mar 8, 2022

Create the option of adding the certificate to already available Azure AD App using the command aad app set.
Following would be the expected options for the command.

Option Description
--certificateFile [certificateFile] Path to the file with certificate private key. Specify either certificateFile or certificateBase64Encoded
--certificateBase64Encoded [certificateBase64Encoded] Base64-encoded string with certificate private key. Specify either certificateFile or certificateBase64Encoded
--certificateDisplayName [certificateDisplayName] Display name for the certificate. If not given, the displayName will be set to the certificate subject
@martinlingstuyl
Copy link
Contributor

Hi guys, I like this one, you can assign me here!

@arjunumenon
Copy link
Member Author

Thanks @martinlingstuyl for your help.
All yours!!

@martinlingstuyl
Copy link
Contributor

Hi @arjunumenon,

would it be an idea to add an extra option: --certificateDisplayName?

Using the displayName property on the Microsoft Graph AAD app update call is optional. If not filled in, the displayName will be set to the certificate subject.
In my case it became something like: CN=Test Certificate for CLI

@waldekmastykarz
Copy link
Member

Makes perfect sense @martinlingstuyl. Great suggestion! 👏

@arjunumenon
Copy link
Member Author

Wonderful suggestion @martinlingstuyl 👍. I have updated the spec for the same.

I have also updated the spec for #3115 which also deals with the similar enhancement

@martinlingstuyl
Copy link
Contributor

Fantastic, I'm already working on it. Expect a pull request in a few days!

@martinlingstuyl
Copy link
Contributor

A small thing. The specs speak about a private key, but I assume you mean public key...

@martinlingstuyl
Copy link
Contributor

Hi guys, is it acceptable if I close #3115 and #3116 in a single PR?

@waldekmastykarz
Copy link
Member

Hi guys, is it acceptable if I close #3115 and #3116 in a single PR?

Yes, but please keep the commits separate so that it's clear which changes apply where.

@martinlingstuyl
Copy link
Contributor

By the way, I'm not replacing the certificate. I'm adding it to the list of available certificates. Seems to me the best route, as you would want to have some form of fallback to use the older certificates if they are referenced somewhere.

@waldekmastykarz
Copy link
Member

That's definitely the right approach: let's not replace whatever is in there but rather add another certificate

@martinlingstuyl
Copy link
Contributor

This can be closed now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants