Adds `LicensingService` #40

justindbaur · 2024-11-01T20:54:03Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-12643

📔 Objective

Proposal of a new `ILicensingService`:

This adds a ILicensingService to this library. It differs heavily from its server counterpart. An explanation of its interface:

IsCloud, this service now becomes the arbiter of whether the running service is running in the cloud or is a self-hosted service and this property is the way to check that. It does this not through a simple selfHosted config key but through a capabilities check. It calls itself cloud if it has license signing abilities and it does this by ensuring it has a Bitwarden sanctioned certificate and that that certificate has a private key. It will return false when it has a Bitwarden sanctioned certificate without a private key.
string CreateLicense(IEnumerable<Claim>, TimeSpan) this is the new method for creating a license, this license can then be packaged into a file and downloaded by a user for a later upload into their self-hosted server. This method takes only two things, a list of claims, which are just a simple key value pair of data that you want packaged into the license so that it can be consumed later when the license is validated. You also give a TimeSpan which is how long your license should be valid for.
Task<IEnumerable<Claim>> VerifyLicenseAsync(string) this is the method for consuming the license file. You give it the original license contents and, if valid, it will give you back the original claims that you gave it on the creation side. With this response you can look for and retrieve capabilities that were bestowed on that license from cloud. With proper defensive programming there should never be a license versioning problem like we've had in the past. The types of values stored in here have no rules, you can stored a value that is serialized json so that you can store complex information in the license. The license is a JWT token as apposed to the current style which is a JSON file. The contents of the license are not as simply viewable (i.e just opening the file in any IDE program) but they are still viewable with JWT tools like jwt.io the contents should not be considered secret.

We will not be able to simply switch over to this new licensing service in server. It will require a long migration but it is a migration we can do. All new license files that come from Bitwarden should probably switch to this format.

The responsibility of building the initial list of claims and reading the claims is up to the feature team that needs to make use of the license.

Points of interest

This doesn't currently remove all reliance on the globalSettings:selfHosted config key. I want it to but I struggle to remove its final usage, which is being a flag to decide whether or not to read in the appsettings.SelfHosted.json file.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

codecov · 2024-11-01T20:55:33Z

Codecov Report

Attention: Patch coverage is 70.66667% with 66 lines in your changes missing coverage. Please review.

Project coverage is 43.14%. Comparing base (756d31c) to head (04c405f).

Files with missing lines	Patch %	Lines
...ing/src/Licensing/PostConfigureLicensingOptions.cs	68.31%	24 Missing and 8 partials ⚠️
...en.Extensions.Hosting/src/HostBuilderExtensions.cs	0.00%	24 Missing and 1 partial ⚠️
...s.Hosting/src/Licensing/InvalidLicenseException.cs	45.45%	6 Missing ⚠️
...s.Hosting/src/Licensing/DefaultLicensingService.cs	95.89%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
+ Coverage   37.88%   43.14%   +5.25%     
==========================================
  Files          38       41       +3     
  Lines        1127     1335     +208     
  Branches       99      123      +24     
==========================================
+ Hits          427      576     +149     
- Misses        666      715      +49     
- Partials       34       44      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

withinfocus

I love the direction change and the requisite changes. It'll be work in server to use and that's its only implementation, but it makes me wonder about where else we'd engage this. Could it be used for our independent components like Key Connector, and we support that in the creation and validation routines as "products", or are those just claims?

I would like to understand the backstory with appsettings.SelfHosted.json and if we could indeed drop it entirely.

Besides a few to-dos this seems good to merge to me.

withinfocus · 2024-11-05T20:41:32Z

extensions/Bitwarden.Extensions.Hosting/src/Licensing/DefaultLicensingService.cs

+        ArgumentNullException.ThrowIfNull(logger);
+        ArgumentNullException.ThrowIfNull(internalLicensingOptions);
+
+        // TODO: Do we need to support runtime changes to these settings at all, I don't think we do...


withinfocus · 2024-11-05T20:46:17Z

extensions/Bitwarden.Extensions.Hosting/src/Licensing/DefaultLicensingService.cs

+    public async Task<IEnumerable<Claim>> VerifyLicenseAsync(string license)
+    {
+        ArgumentNullException.ThrowIfNull(license);
+        // TODO: Should we validate that this is self host?


ℹ️ I'd say no. I can make up some ideas around how we'd offer license verification capabilities somewhere new so we don't need to constrain this.

Sweet, I concur.

withinfocus · 2024-11-05T20:48:56Z

extensions/Bitwarden.Extensions.Hosting/src/Licensing/DefaultLicensingService.cs

+        }
+
+        // Should I even take a ClaimsIdentity and return it here instead of a list of claims?
+        return tokenValidationResult.ClaimsIdentity.Claims;


ℹ️ I took a look and don't see anything else we'd want to return really.

withinfocus · 2024-11-05T20:49:55Z

extensions/Bitwarden.Extensions.Hosting/src/Licensing/ILicensingService.cs

+    /// </summary>
+    bool IsCloud { get; }
+
+    // TODO: Any other options than valid for?


💭 Limit the product names enabled perhaps? Made up constraint by me.

I personally don't want to limit the product names in anyway, if we did it would require that we update this library whenever we create a new product, which feels like an artificial slowdown. It would also do nothing to stop one product from using another products name. But I do think that maybe taking the product name in this method could be a good idea. Right now, this has a limitation of creating licenses for only a single products licenses per project. Maybe that is a limitation we don't want. Just adding the product name here though would still have all products share the same signing certificate which maybe we'd want that configurable per product too.

But forcing the product name to be passed in here does force them to configure it, vs now I allow it to be configured but grab the IHostEnvironment.ApplicationName (csproj name) which could have someone rename it one day without knowing the ramifications.

Also, I think I am going back on accepting a TimeSpan now and think it should be a DateTime. Most licenses are going to be valid for longer than a month, most likely in terms of years and their aren't TimeSpan.FromYears overloads (because a year is variable based on when you start it), likely expiration date is going to come from something like stripe instead of being something you can hardcode and I'm pretty sure it will come from stripe as a DateTime. Thoughts?

I'm not opposed to updating this when a product is added -- that's a major event that requires preparation. That said, it has pitfalls and I am just making this up so we don't need to add it; if we want to license differently in the future we can figure it out then.

Renaming a .csproj seems a bit weird to me and I figure we don't need to account for it.

I like the idea of allowing a set time to be the input instead. Let the caller figure out when they want it to expire.

withinfocus · 2024-11-05T20:52:50Z

extensions/Bitwarden.Extensions.Hosting/src/Licensing/PostConfigureLicensingOptions.cs

@@ -0,0 +1,177 @@
+using System.Reflection;
+using System.Security.Cryptography.X509Certificates;
+using Azure.Storage.Blobs;


💭 I cringe a bit that we're adding this large dependency to the library and hope it has some reuse.

Yeah, same. This is partly why I want to make sure this library is trimmable so that the parts you don't need could be trimmed away but the alternative I have been considering is turning this into an SDK. So instead of configuring what you need through BitwardenHostOptions you do it through MSBuild properties like <EnableCloudLicensingSupport>true</EnableCloudLicensingSupport> and this way you only take on this dependency if you actually use it. It would likely be a bit more development effort but doable.

Hmm, trimming seems sufficient to me, and wouldn't require us to ship differences in build products right?

withinfocus · 2024-11-05T20:53:55Z

extensions/Bitwarden.Extensions.Hosting/src/Licensing/PostConfigureLicensingOptions.cs

+            }
+        }
+
+


⛏️ Extra newlines that trigger me.

withinfocus · 2024-11-05T20:55:27Z

extensions/Bitwarden.Extensions.Hosting/src/Licensing/PostConfigureLicensingOptions.cs

+        // Try Cert store
+        if (TryCertStore(options))
+        {
+            DoFinalValidation();
+            return;
+        }
+
+
+        // Try Assembly embedded
+        if (TryGetEmbeddedCert(options))
+        {
+            DoFinalValidation();
+            return;
+        }


❓ What's the history with these? Not sure I know their use.

It's essentially this logic: https://github.com/bitwarden/server/blob/dae493db72b10a4cbccb0478a52bea1eb6250630/src/Core/Services/Implementations/LicensingService.cs#L47-L68 except for a few key changes.

First, I don't do any "are you self-hosted?" checks. I just check all the locations no matter what and at the end of it if I have a valid cert with a private key, you can be cloud. Also, I changed the order of checking from azure -> embedded -> cert store to the above. That's because I did away with the self hosted checks and since even our cloud builds have the embedded cert, I needed to prefer the cert store.

But we very likely could do away with the cert store. I just can't say for sure that it isn't used but it also doesn't add any dependencies.

My question was really "is this still used?" but I guess with just a few lines of code and no new dependencies it doesn't hurt to leave it.

withinfocus · 2024-11-05T20:56:20Z

extensions/Bitwarden.Extensions.Hosting/tests/XUnitLoggerProvider.cs

+    }
+}
+
+// Ref: https://github.com/dotnet/aspnetcore/blob/main/src/Testing/src/Logging/XunitLoggerProvider.cs


❓ These haven't been packaged up by someone?

Looks like yes actually, would you prefer I use this? https://github.com/martincostello/xunit-logging

This is subjective, but yeah that looks like it's popular and active so I'd prefer we outsource it vs. have our own and likely miss out on stuff, including maintenance.

github-actions · 2024-11-07T07:06:02Z

Checkmarx One – Scan Summary & Details – d1111b75-01a1-4108-934c-935d92f4f3cc

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Heap_Inspection	/extensions/Bitwarden.Extensions.Hosting/tests/Licensing/TestData.cs: 212	Attack Vector
	Password_In_Comment	/extensions/Bitwarden.Extensions.Hosting/tests/Licensing/TestData.cs: 8	Attack Vector

withinfocus

I see a few other to-dos -- merge now and solve later or what?

withinfocus · 2024-11-07T13:28:34Z

extensions/Bitwarden.Extensions.Hosting/src/Licensing/LicensingOptions.cs

+    /// </summary>
+    public string? CertificatePassword { get; set; }
+
+    // TODO: Do we actually need to allow these to be customized?


ℹ️ Missed this ... I think no.

justindbaur added 9 commits October 28, 2024 23:04

Work On Licensing

14e878b

Add Azure Support & Tests

42d5b7b

Embed Certs

5448139

Add AzureBlob Tests

aede3ce

Change SelfHostedAttribute to Use LicensingService

b08a2ab

Formatting

e49220d

Make Licensing Optional

7dd8a28

Merge remote-tracking branch 'origin/main' into self-host-support

43eb08c

Finish DefaultLicensingService Tests

2cc8f7a

justindbaur added 2 commits November 2, 2024 11:13

Do Require Issuer & Audience

2abf460

Add Tests For Issuer & Audience Validation

bdf7a03

withinfocus reviewed Nov 5, 2024

View reviewed changes

justindbaur added 3 commits November 5, 2024 21:09

PR Feedback

5597d20

Use DateTime For Expiration

8154ce5

Remove Extra Space

04c405f

withinfocus reviewed Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `LicensingService` #40

Adds `LicensingService` #40

justindbaur commented Nov 1, 2024 •

edited by withinfocus

Loading

codecov bot commented Nov 1, 2024 •

edited

Loading

withinfocus left a comment

withinfocus Nov 5, 2024

withinfocus Nov 5, 2024

justindbaur Nov 6, 2024

withinfocus Nov 5, 2024

withinfocus Nov 5, 2024

justindbaur Nov 6, 2024

withinfocus Nov 6, 2024

withinfocus Nov 5, 2024

justindbaur Nov 6, 2024

withinfocus Nov 6, 2024

withinfocus Nov 5, 2024

withinfocus Nov 5, 2024

justindbaur Nov 6, 2024

withinfocus Nov 6, 2024

withinfocus Nov 5, 2024

justindbaur Nov 6, 2024

withinfocus Nov 6, 2024

github-actions bot commented Nov 7, 2024

withinfocus left a comment

withinfocus Nov 7, 2024

Adds LicensingService #40

Are you sure you want to change the base?

Adds LicensingService #40

Conversation

justindbaur commented Nov 1, 2024 • edited by withinfocus Loading

🎟️ Tracking

📔 Objective

Proposal of a new ILicensingService:

Points of interest

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented Nov 1, 2024 • edited Loading

Codecov Report

withinfocus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 7, 2024

New Issues

withinfocus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adds `LicensingService` #40

Adds `LicensingService` #40

justindbaur commented Nov 1, 2024 •

edited by withinfocus

Loading

Proposal of a new `ILicensingService`:

codecov bot commented Nov 1, 2024 •

edited

Loading