Skip to content

Commit 71f95a0

Browse files
SilvengaSilvenga
committed
Improved upgrade path for older TLS certificate generation versions.
1 parent a0dfef9 commit 71f95a0

File tree

7 files changed

+181
-31
lines changed

7 files changed

+181
-31
lines changed

‎src/Contrast.K8s.AgentOperator/Core/Tls/TlsCertificateChainConverter.cs‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
// Contrast Security, Inc licenses this file to you under the Apache 2.0 License.
22
// See the LICENSE file in the project root for more information.
33

4-
using System;
54
using System.IO;
65
using System.Security.Cryptography.X509Certificates;
76
using System.Text;
@@ -18,8 +17,6 @@ public interface ITlsCertificateChainConverter
1817

1918
public class TlsCertificateChainConverter : ITlsCertificateChainConverter
2019
{
21-
private const byte GenerationVersion = 2;
22-
2320
public TlsCertificateChainExport Export(TlsCertificateChain chain)
2421
{
2522
var caCertificatePem = chain.CaCertificate.Export(X509ContentType.Pkcs12);
@@ -29,22 +26,17 @@ public TlsCertificateChainExport Export(TlsCertificateChain chain)
2926

3027
var serverCertificatePem = chain.ServerCertificate.Export(X509ContentType.Pkcs12);
3128

32-
return new TlsCertificateChainExport(caCertificatePem, caPublicPem, serverCertificatePem, new[]{ GenerationVersion });
29+
return new TlsCertificateChainExport(caCertificatePem, caPublicPem, serverCertificatePem, chain.Version);
3330
}
3431

3532
public TlsCertificateChain Import(TlsCertificateChainExport export)
3633
{
3734
var (caCertificatePem, _, serverCertificatePem, version) = export;
3835

39-
if (version.Length != 1 || version[0] != GenerationVersion)
40-
{
41-
throw new Exception($"Incompatible generated version '{string.Join(",", version)}' detected, expected '{GenerationVersion}'.");
42-
}
43-
4436
var caCertificate = new X509Certificate2(caCertificatePem, (string?)null, X509KeyStorageFlags.Exportable);
4537
var serverCertificate = new X509Certificate2(serverCertificatePem, (string?)null, X509KeyStorageFlags.Exportable);
4638

47-
return new TlsCertificateChain(caCertificate, serverCertificate);
39+
return new TlsCertificateChain(caCertificate, serverCertificate, version);
4840
}
4941

5042
private static byte[] CreatePem(object o)

‎src/Contrast.K8s.AgentOperator/Core/Tls/TlsCertificateChainGenerator.cs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ public interface ITlsCertificateChainGenerator
1919

2020
public class TlsCertificateChainGenerator : ITlsCertificateChainGenerator
2121
{
22+
public static readonly byte[] GenerationVersion = { 3 };
23+
2224
private readonly CreateCertificates _createCertificates;
2325
private readonly TlsCertificateOptions _options;
2426

@@ -33,7 +35,7 @@ public TlsCertificateChain CreateTlsCertificateChain()
3335
var ca = CreateRootCa(_options);
3436
var serverCertificate = CreateServerCertificate(ca);
3537

36-
return new TlsCertificateChain(ca, serverCertificate);
38+
return new TlsCertificateChain(ca, serverCertificate, GenerationVersion);
3739
}
3840

3941
private X509Certificate2 CreateRootCa(TlsCertificateOptions options)
@@ -135,7 +137,7 @@ private X509Certificate2 CreateServerCertificate(X509Certificate2 signingCertifi
135137
};
136138
}
137139

138-
public record TlsCertificateChain(X509Certificate2 CaCertificate, X509Certificate2 ServerCertificate) : IDisposable
140+
public record TlsCertificateChain(X509Certificate2 CaCertificate, X509Certificate2 ServerCertificate, byte[] Version) : IDisposable
139141
{
140142
public void Dispose()
141143
{

‎src/Contrast.K8s.AgentOperator/Core/Tls/TlsCertificateChainValidator.cs‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
// Contrast Security, Inc licenses this file to you under the Apache 2.0 License.
2+
// See the LICENSE file in the project root for more information.
3+
4+
using System;
5+
using System.Linq;
6+
7+
namespace Contrast.K8s.AgentOperator.Core.Tls
8+
{
9+
public interface ITlsCertificateChainValidator
10+
{
11+
bool IsValid(TlsCertificateChain chain, out ValidationResultReason reason);
12+
}
13+
14+
public class TlsCertificateChainValidator : ITlsCertificateChainValidator
15+
{
16+
public bool IsValid(TlsCertificateChain chain, out ValidationResultReason reason)
17+
{
18+
var renewThreshold = DateTime.Now + TimeSpan.FromDays(90);
19+
20+
if (!chain.CaCertificate.HasPrivateKey
21+
|| !chain.ServerCertificate.HasPrivateKey)
22+
{
23+
reason = ValidationResultReason.MissingPrivateKey;
24+
}
25+
else if (chain.CaCertificate.NotAfter < renewThreshold
26+
|| chain.ServerCertificate.NotAfter < renewThreshold)
27+
{
28+
reason = ValidationResultReason.Expired;
29+
}
30+
else if (!TlsCertificateChainGenerator.GenerationVersion.SequenceEqual(chain.Version))
31+
{
32+
reason = ValidationResultReason.OldVersion;
33+
}
34+
else
35+
{
36+
reason = ValidationResultReason.NoError;
37+
}
38+
39+
return reason == ValidationResultReason.NoError;
40+
}
41+
}
42+
43+
public enum ValidationResultReason
44+
{
45+
NoError = 0,
46+
MissingPrivateKey,
47+
Expired,
48+
OldVersion
49+
}
50+
}

‎src/Contrast.K8s.AgentOperator/Core/Tls/TlsCertificateMaintenanceHandler.cs‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -22,24 +22,29 @@ public class TlsCertificateMaintenanceHandler : INotificationHandler<EntityRecon
2222
private readonly ITlsCertificateChainConverter _certificateChainConverter;
2323
private readonly IKubeWebHookConfigurationWriter _webHookConfigurationWriter;
2424
private readonly IWebHookSecretParser _webHookSecretParser;
25+
private readonly ITlsCertificateChainValidator _validator;
2526

2627
public TlsCertificateMaintenanceHandler(IKestrelCertificateSelector certificateSelector,
2728
ITlsCertificateChainGenerator certificateChainGenerator,
2829
ITlsCertificateChainConverter certificateChainConverter,
2930
IKubeWebHookConfigurationWriter webHookConfigurationWriter,
30-
IWebHookSecretParser webHookSecretParser)
31+
IWebHookSecretParser webHookSecretParser,
32+
ITlsCertificateChainValidator validator)
3133
{
3234
_certificateSelector = certificateSelector;
3335
_certificateChainGenerator = certificateChainGenerator;
3436
_certificateChainConverter = certificateChainConverter;
3537
_webHookConfigurationWriter = webHookConfigurationWriter;
3638
_webHookSecretParser = webHookSecretParser;
39+
_validator = validator;
3740
}
3841

3942
public Task Handle(EntityReconciled<V1Secret> notification, CancellationToken cancellationToken)
4043
{
4144
if (_webHookSecretParser.TryGetWebHookCertificateSecret(notification.Entity, out var chain))
4245
{
46+
// The certificate may not be valid, but that's not for us to figure out right now.
47+
// At this point, we only care if the certificate was parseable.
4348
if (_certificateSelector.TakeOwnershipOfCertificate(chain))
4449
{
4550
Logger.Info($"Secret '{notification.Entity.Namespace()}/{notification.Entity.Name()}' was changed, updated internal certificates.");
@@ -58,27 +63,31 @@ public async Task Handle(LeaderStateChanged notification, CancellationToken canc
5863
if (notification.IsLeader)
5964
{
6065
var existingSecret = await _webHookConfigurationWriter.FetchCurrentCertificate();
61-
if (existingSecret == null)
66+
if (existingSecret == null
67+
|| !_webHookSecretParser.TryGetWebHookCertificateSecret(existingSecret, out var chain))
6268
{
6369
// Missing.
6470
await GenerateAndPublishCertificate();
6571
}
66-
else if (_webHookSecretParser.TryGetWebHookCertificateSecret(existingSecret, out var chain))
72+
else
6773
{
6874
using (chain)
6975
{
70-
// Existing and valid, ensure web hook ca bundle is okay.
71-
Logger.Info("Web hook certificate secret is valid.");
72-
var chainExport = _certificateChainConverter.Export(chain);
73-
await _webHookConfigurationWriter.UpdateClusterWebHookConfiguration(chainExport);
76+
if (_validator.IsValid(chain, out var reason))
77+
{
78+
// Existing and valid, ensure web hook ca bundle is okay.
79+
Logger.Info("Web hook certificate secret is valid.");
80+
var chainExport = _certificateChainConverter.Export(chain);
81+
await _webHookConfigurationWriter.UpdateClusterWebHookConfiguration(chainExport);
82+
}
83+
else
84+
{
85+
// Invalid.
86+
Logger.Info($"Web hook certificate secret is invalid (Reason: '{reason}').");
87+
await GenerateAndPublishCertificate();
88+
}
7489
}
7590
}
76-
else
77-
{
78-
// Invalid.
79-
Logger.Info("Web hook certificate secret is invalid.");
80-
await GenerateAndPublishCertificate();
81-
}
8291
}
8392
}
8493

‎src/Contrast.K8s.AgentOperator/Core/Tls/WebHookSecretParser.cs‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,18 @@ public bool TryGetWebHookCertificateSecret(V1Secret secret, [NotNullWhen(true)]
3535
&& secret.Data != null
3636
&& secret.Data.TryGetValue(_tlsStorageOptions.ServerCertificateName, out var serverCertificateBytes)
3737
&& secret.Data.TryGetValue(_tlsStorageOptions.CaPublicName, out var caPublicBytes)
38-
&& secret.Data.TryGetValue(_tlsStorageOptions.CaCertificateName, out var caCertificateBytes)
39-
&& secret.Data.TryGetValue(_tlsStorageOptions.VersionName, out var versionBytes))
38+
&& secret.Data.TryGetValue(_tlsStorageOptions.CaCertificateName, out var caCertificateBytes))
4039
{
40+
// Version is newer, so we might be upgrading from a version without versions.
41+
var version = Array.Empty<byte>();
42+
if (secret.Data.TryGetValue(_tlsStorageOptions.VersionName, out var versionBytes))
43+
{
44+
version = versionBytes;
45+
}
46+
4147
try
4248
{
43-
chain = _certificateChainConverter.Import(new TlsCertificateChainExport(caCertificateBytes, caPublicBytes, serverCertificateBytes, versionBytes));
49+
chain = _certificateChainConverter.Import(new TlsCertificateChainExport(caCertificateBytes, caPublicBytes, serverCertificateBytes, version));
4450

4551
var renewThreshold = DateTime.Now + TimeSpan.FromDays(90);
4652
return chain.CaCertificate.HasPrivateKey

‎tests/Contrast.K8s.AgentOperator.Tests/Core/Tls/TlsCertificateChainValidatorTests.cs‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Contrast Security, Inc licenses this file to you under the Apache 2.0 License.
2+
// See the LICENSE file in the project root for more information.
3+
4+
using System;
5+
using AutoFixture;
6+
using CertificateManager;
7+
using Contrast.K8s.AgentOperator.Core.Tls;
8+
using Contrast.K8s.AgentOperator.Options;
9+
using FluentAssertions;
10+
using FluentAssertions.Execution;
11+
using Xunit;
12+
13+
namespace Contrast.K8s.AgentOperator.Tests.Core.Tls
14+
{
15+
public class TlsCertificateChainValidatorTests
16+
{
17+
private static readonly Fixture AutoFixture = new();
18+
19+
[Fact]
20+
public void When_chain_expiration_is_valid_then_IsValid_should_return_true()
21+
{
22+
using var chainFake = FakeCertificates(TimeSpan.FromDays(180));
23+
24+
var validator = new TlsCertificateChainValidator();
25+
26+
// Act
27+
var result = validator.IsValid(chainFake, out _);
28+
29+
// Assert
30+
result.Should().BeTrue();
31+
}
32+
33+
[Fact]
34+
public void When_chain_expiration_is_expired_then_IsValid_should_return_expired()
35+
{
36+
using var chainFake = FakeCertificates(TimeSpan.FromDays(45));
37+
38+
var validator = new TlsCertificateChainValidator();
39+
40+
// Act
41+
var result = validator.IsValid(chainFake, out var reason);
42+
43+
// Assert
44+
using (new AssertionScope())
45+
{
46+
result.Should().BeFalse();
47+
reason.Should().Be(ValidationResultReason.Expired);
48+
}
49+
}
50+
51+
[Fact]
52+
public void When_chain_version_differs_then_IsValid_should_return_old_version()
53+
{
54+
using var chainFake = FakeCertificates(TimeSpan.FromDays(180)) with
55+
{
56+
Version = AutoFixture.Create<byte[]>()
57+
};
58+
59+
var validator = new TlsCertificateChainValidator();
60+
61+
// Act
62+
var result = validator.IsValid(chainFake, out var reason);
63+
64+
// Assert
65+
using (new AssertionScope())
66+
{
67+
result.Should().BeFalse();
68+
reason.Should().Be(ValidationResultReason.OldVersion);
69+
}
70+
}
71+
72+
private static TlsCertificateChain FakeCertificates(TimeSpan expiresAfter)
73+
{
74+
var generator = new TlsCertificateChainGenerator(
75+
new CreateCertificates(new CertificateUtility()),
76+
AutoFixture.Create<TlsCertificateOptions>() with { ExpiresAfter = expiresAfter }
77+
);
78+
return generator.CreateTlsCertificateChain();
79+
}
80+
}
81+
}

‎tests/Contrast.K8s.AgentOperator.Tests/Core/Tls/TlsCertificateMaintenanceHandlerTests.cs‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,18 @@ public async Task When_leader_elected_and_secret_is_valid_then_cluster_web_hook_
6161
var converterMock = Substitute.For<ITlsCertificateChainConverter>();
6262
converterMock.Export(chainFake).Returns(exportFake);
6363

64+
var validatorMock = Substitute.For<ITlsCertificateChainValidator>();
65+
validatorMock.IsValid(Arg.Is(chainFake), out _).Returns(info =>
66+
{
67+
info[1] = ValidationResultReason.NoError;
68+
return true;
69+
});
70+
6471
var handler = CreateGraph(
6572
webHookSecretParser: parserMock,
6673
webHookConfigurationWriter: writerMock,
67-
certificateChainConverter: converterMock
74+
certificateChainConverter: converterMock,
75+
certificateChainValidator:validatorMock
6876
);
6977

7078
// Act
@@ -143,14 +151,16 @@ private static TlsCertificateMaintenanceHandler CreateGraph(IKestrelCertificateS
143151
ITlsCertificateChainGenerator? certificateChainGenerator = null,
144152
ITlsCertificateChainConverter? certificateChainConverter = null,
145153
IKubeWebHookConfigurationWriter? webHookConfigurationWriter = null,
146-
IWebHookSecretParser? webHookSecretParser = null)
154+
IWebHookSecretParser? webHookSecretParser = null,
155+
ITlsCertificateChainValidator? certificateChainValidator = null)
147156
{
148157
return new TlsCertificateMaintenanceHandler(
149158
certificateSelector ?? Substitute.For<IKestrelCertificateSelector>(),
150159
certificateChainGenerator ?? Substitute.For<ITlsCertificateChainGenerator>(),
151160
certificateChainConverter ?? Substitute.For<ITlsCertificateChainConverter>(),
152161
webHookConfigurationWriter ?? Substitute.For<IKubeWebHookConfigurationWriter>(),
153-
webHookSecretParser ?? Substitute.For<IWebHookSecretParser>()
162+
webHookSecretParser ?? Substitute.For<IWebHookSecretParser>(),
163+
certificateChainValidator ?? Substitute.For<ITlsCertificateChainValidator>()
154164
);
155165
}
156166

0 commit comments

Comments
 (0)