search

microsoft / ALAppExtensions

Repository for collaboration on Microsoft AL application add-on and localization extensions for Microsoft Dynamics 365 Business Central.
MIT License
763 stars 608 forks source link

AcquireTokensWithCertificate() with password #22261

Closed jwikman closed 6 months ago

jwikman commented 1 year ago

We are working on an addition to the SharePoint Authorization module, to also support Service to Service / Client Credentials authentication for SharePoint.

It turned out that SharePoint does not work with Client Credentials tokens that are generated from Client Id and Client Secret. SharePoint API only supports Client Credential tokens that are generated from Client Id and a Certificate.

We have this function to request a token with a certificate: https://github.com/microsoft/ALAppExtensions/blob/9151fa39d6f6d548de0acdcf67512a30baf297ba/Modules/System/OAuth2/OAuth2Impl.Codeunit.al#L584 BUT, this function do not accept a password for the certificate. The certificate to use should be a PKCS#12 certificate with private keys, most often saved to file as a .pfx file. It is a security best practice to protect the .pfx file with a password, and in most certificate generators the password is mandatory - which make sense!

I can see that the only usage of AcquireTokensWithCertificate() in the Base App is where the certificate is retrieved from a KeyVault, and in that case a password is not necessary - so I understand why the current implementation is as it is.

But I would like to request an overload of AcquireTokensWithCertificate() that also has a CertificatePassword (text), that could be passed to AuthFlow.ALAcquireApplicationTokensWithCertificate() and further to the X509 Certificate that probably is used in C# somewhere. ;-)

When that is in place, I'll create a PR (with some docs, because this is not straight forward) to add Client Credentials to SharePoint Authorization module I don't think that we should add this without the usage of a password protected pfx. We only want to promote security best practices, don't you agree @JesperSchulz?

jwikman commented 1 year ago

I've added a draft PR for the functionality we want to add, #22404, so you can see what we want to accomplish.

jwikman commented 1 year ago

I just realized that also the AcquireTokensFromCacheWithCertificate() and AuthFlow.ALAcquireTokensFromCacheWithCertificate() functions needs to be overloaded with a certificate password.

jwikman commented 1 year ago

@IhorHandziuk what do you think about this request? Would it be possible to implement anytime soon? If you don't know, maybe you know who does?

JesperSchulz commented 1 year ago

Hi Johannes! This does indeed sound like a very valid request. I cannot quite wrap my mind around the size of the required work though, and will hence add two of our security experts to this issue. @darjoo and @WaelAbuSeada, what would you gentlemen think? Is this something we can provide with a reasonable investment and within a reasonable timeframe, or is this a larger effort which must get prioritized against other work in our backlog?

WaelAbuSeada commented 1 year ago

Hi Johannes! This seems like a fair ask, we need to update the platform binaries then we can add the support in the system module, so yes we are working on it :)

jwikman commented 1 year ago

@WaelAbuSeada, that was good news! Just let me know if you need anything from me.

(I was writing on a "request-for-clarification" reply on your un-edited post first, but the edit was indeed good news 😀)

jwikman commented 1 year ago

@WaelAbuSeada I hope you noticed my addition with AcquireTokensFromCacheWithCertificate() and AuthFlow.ALAcquireTokensFromCacheWithCertificate() as well, so they get included in the first round. :)

JesperSchulz commented 1 year ago

Thanks for reporting this. We agree, and we’ll publish a fix asap, either in an update for the current version or in the next major release. Please do not reply to this, as we do not monitor closed issues. If you have follow-up questions or requests, please create a new issue where you reference this one.

Build ID: 23.0.10594.0.

JesperSchulz commented 1 year ago

Availability update: We will publish a fix for this issue in the next update for release 22.

Build ID to track: 22.0.57440.0.

pri-kise commented 1 year ago

@JesperSchulz i checked the latest BC 23 dll (Microsoft.Dynamics.Nav.Ncl.dll) for public class ALAzureAdCodeGrantFlow, but I can't find the new method, that accepts a password. I only find the procedure ALAcquireApplicationTokensWithCertificate (without password).

And I found some overloads for the method: ALAcquireTokensFromCacheWithCertificate that accepts a password, but we need this for the normal procedure, too.

JesperSchulz commented 1 year ago

Looping in @WaelAbuSeada for more details on the provided solution.

pri-kise commented 1 year ago

I'm referring to these methods in the DLL: image

image

JesperSchulz commented 9 months ago

@pri-kise, was this ever resolved?

jwikman commented 9 months ago

@JesperSchulz I just checked both the AL code (codeunit 502 OAuth2Impl) and Microsoft.Dynamics.Nav.Ncl.dll, BC23.1. But I cannot find any trace of that this should've been resolved. Please correct me if I'm wrong, because I would be very happy if I was wrong on this! 😉

I believe that this one should be reopened and looked into.

I talked to a colleague this morning that finally had some time to look into using this for the SharePoint module (https://github.com/microsoft/ALAppExtensions/pull/22404), but I guess that's blocked by this issue then.

pri-kise commented 8 months ago

I must agree with @jwikman.

I cannot confirm, that this issue was ever resolved.

JesperSchulz commented 8 months ago

That's what I thought. These closed issues have a tendency of falling between the cracks when transferred to other teams. This will have to get reactivated.

jwikman commented 7 months ago

@JesperSchulz, any news to share on this?

JesperSchulz commented 7 months ago

I'll ping again. The bug is still active. I'll try to get someone to come with an update by end of week 🤞

JesperSchulz commented 7 months ago

I'll get one of our engineers to look into this once more. I hope we can get something done about this for 2024 wave 1!

jwikman commented 7 months ago

Ok, thanks @JesperSchulz!

JesperSchulz commented 7 months ago

Work for this is now in progress!

jwikman commented 7 months ago

Work for this is now in progress!

Thanks!

darjoo commented 7 months ago

I'll add that, the function has been added to platform. I'll add it to app soon.

darjoo commented 7 months ago

@jwikman Do take a look at this PR which I've added it. https://github.com/microsoft/BCApps/pull/471

It is in the BCApps repo and part of my other work which is uptaking of SecretText within the System Application.

pri-kise commented 7 months ago

@jwikman Do take a look at this PR which I've added it. microsoft/BCApps#471

It is in the BCApps repo and part of my other work which is uptaking of SecretText within the System Application.

@darjoo may it be that a public codeunit AcquireTokenAndTokenCacheByAuthorizationCodeWithCertificate is missing that accepts a Certificate Password?

jwikman commented 7 months ago

@jwikman Do take a look at this PR which I've added it.

@darjoo I think that should do the trick for that function.

But I think that there are more certificate functions in that codeunit that should get a CertificatePassword parameter as well. For example AcquireTokensFromCacheWithCertificate() as mentioned in https://github.com/microsoft/ALAppExtensions/issues/22261#issuecomment-1447863135 as well as AcquireTokenAndTokenCacheByAuthorizationCodeWithCertificate mentioned in https://github.com/microsoft/ALAppExtensions/issues/22261#issuecomment-1903959085. There are quite a few *WithCertificate functions that all probably would gain from a CertificatePasswordparameter...

Since you are changing the signature for all functions anyway, couldn't it be a good opportunity to add the CertificatePassword as a mandatory parameter to all those *WithCertificate functions? And a blank password should then be counted as no password...

pri-kise commented 7 months ago

@jwikman could you check the PR of @darjoo again?

I think he has added now the required procedures.

Are you having the time to create an updated PR for BCApps Repository in the near future (for BC24?)? Otherwise I could try to help you on this, but I wouldn't know how to create a test for this..

jwikman commented 7 months ago

@darjoo I added a few comments, where the CertificatePassword parameter was not used in all places.

@pri-kise, sadly enough, I don't think I'll manage to get the time to do this in time for BC24... I've already promised to do too much for this wave 😅

I can offer help with CR if you have the possibility to give it a try! But I do not know what to with the tests for these kind of features... I will also be sure to test it out IRL as soon as it is available in the preview sandboxes.

pri-kise commented 7 months ago

@jwikman I used you draft PR to create a PR for the BCApps Repository.

My branch is currently based on the @darjoo's PR.

You can see the changes in the last Commit there: https://github.com/microsoft/BCApps/pull/525/commits/c2a4e4c9fd2d9e1aec6dea37a8e0965492a37b77

I'd like to test it at least manually, but I don't know how to create a certifacte. I've only worked with ClientId and ClientSecret till now.

jwikman commented 7 months ago

I'd like to test it at least manually, but I don't know how to create a certifacte. I've only worked with ClientId and ClientSecret till now.

Cool! Let me know when you're ready for CR.

In the past I've been using openssl when working with certificates, but I think I found a way to create a password protected .pfx file without installing local tools:

  1. Create a KeyVault in the Azure Portal (or use an existing if you have one)
  2. Generate a new certificate - KeyVault->Certificates->Generate/Import
    • Method: Generate
    • Name: Anything
    • Type: Self-signed
    • Subject: Any X.500 distinguished name
    • Content Type: PKCS #12
  3. After it's been created, go to the certificate version and set Enabled: Yes
  4. Then download the certificate as .pfx from the Certificate Version with the "Download in PFX/PEM format"

Now you've got a PKCS #12 certificate saved as a .pfx file, without password (no good).

To get a password protected .pfx file I did this:

  1. Start Microsoft Management Console by running mmc.exe
  2. Add Snap-In Certificates, select "My user account" when prompted
  3. Expand certificate tree to Personal/Certificates
  4. Import your passwordless .pfx file via Action/All Tasks/Import...
  5. Then right click on your certificate that should be in the Personal/Certificates store and select All Tasks->Export...
  6. Include the Private Key
  7. Set a password when asked
  8. Save to file
  9. Remove the certificate from Personal/Certificates

Now you've got a self-signed PKCS #12 certificate saved as a .pfx file, with a password :)

pri-kise commented 7 months ago

@jwikman thanks for you detailled answer. I was able to generate a certificate with this guide. I uploaded the .cer file that I exported to a I created a small tester app to test the integration manually. I added an action to verfiy the certificate that I upload to my setup in my docker. This seems correct.

https://github.com/pri-kise/msdyn365bc-sharepointclient-test

I fail to retrieve an access token. Maybe it's related to the authority URL that I use or I made a mistake setting up the App Registration.

I receive this warning in my event log of the docker

You must specify only one of the configuration settings AzureActiveDirectoryClientCertificateThumbprint or AzureActiveDirectoryClientSecret. I don't know if this is related.

jwikman commented 7 months ago

These things are soooo painful to debug, since you never get any clues on what is wrong! :/

I assume that you've uploaded the certificate into the app registration, correct? I think you'll need a .cer file for that (without private key), so you might need to export that from MMC as well (or maybe direct from the KeyVault?).

A couple of times I've ran a search&replace in all files and removed all NonDebuggable attributes in the system app to get it a bit more debuggable in my container. Maybe that could help you forward as well?

I do not think that the event log error is related (but not 100% sure...).

Another thing to test is the old AcquireTokensWithCertificate function, where you get the token from a certificate without password. I know that we got things to work with that one when we looked into it last year.

pri-kise commented 7 months ago

Steps I've done as addition to your steps:

Create Azure App Registration: Adding API permissions for Sharepoint (Sites.Read.All)

Furthermore I added the cer file to the App Registration. I Used the MMC to create a cer file.

  1. (Steps to export with private key)
  2. Right click on your certificate that should be in the Personal/Certificates store and select All Tasks->Export...
  3. Not Include the Private Key
  4. Selected DER binary (.cer) as export format
  5. Upload .cer file to azure app registration.

To test this. I Upload the PFX file into my dummy tester app and set the password. I'm able to verify the Certificate, but Authentication keeps failing. I don't know what I can test now, since the Event Log doesn't provide me any help.

jwikman commented 7 months ago

@pri-kise, did you test the AcquireTokensWithCertificate() with a certificate without password?

Just to rule out that it is related to the new functionality with CertificatePassword...

And did I get you correctly, you do not get an AccessToken at all? Or is it when authenticating against SharePoint you get issues?

pri-kise commented 7 months ago

@jwikman I did not get an AccessToken at all (https://github.com/pri-kise/msdyn365bc-sharepointclient-test/blob/main/SharepointClientCred.Codeunit.al; Action TestWithoutPassword)

Yes. I did not get an AccessToken at all.

jwikman commented 7 months ago

Ok. Then it is probably issues with the AppRegistration and/or Scopes, I guess...

Tried to get an AccessToken with the same AppRegistration, but with ClientId + ClientSecret? If you get that to work first, you should also get an AccessToken when using certificate.

pri-kise commented 7 months ago

It's working now. I finally found the solution for my problem. I was using the wrong scope:

I had included a wrong scope in my Scopes. But the errors there could really be improved. Now all methods are working .

I'm going to rebase my branch on the main Branch as a soon as the "uptake secrettext" branch (by @darjoo) is merged. Then the PR is ready for Code Review.

jwikman commented 7 months ago

That is great news @pri-kise!

Well done! 👍

darjoo commented 7 months ago

Great to hear it works! If I find the time, I'll need to try and repro getting the error to see where it is coming from. If we can provide a better message, all the better.

I hope my branch will get in soon too :D Currently still have issues with the platform uptake, so that might be a few more days before resolution.

jwikman commented 7 months ago

Actually, I think I've never got any useful error messages (if any at all!) when it fails to retrieve an Access Token...

It would be very helpful if we got to know that we are using the wrong scopes or something else is wrong with our AppRegistration.

darjoo commented 6 months ago

@jwikman @pri-kise The fix is now merged into the BCApps repo. I shall close this bug now.

I haven't had time to take a look at the error yet, but it is still on my todo-list :)

JesperSchulz commented 6 months ago

Thanks for reporting this. We agree, and we’ll publish a fix asap, either in an update for the current version or in the next major release. Please do not reply to this, as we do not monitor closed issues. If you have follow-up questions or requests, please create a new issue where you reference this one.

Build ID: .

jwikman commented 6 months ago

Great news @darjoo!

@pri-kise it's getting time to publish https://github.com/microsoft/BCApps/pull/525 now then?