Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit c7869af

Browse files
bartonjsbartonjs
committed
Apply code review feedback
* Added TODO items referencing issues for NotImplementedExceptions * Changed LowLevelList<T> usages to List<T> * Added comments as requested.
1 parent 5b0ecc7 commit c7869af

File tree

7 files changed

+72
-22
lines changed

7 files changed

+72
-22
lines changed

‎src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/CertificatePolicy.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ internal sealed class CertificatePolicy
2020
public bool SpecifiedAnyApplicationPolicy { get; set; }
2121
public ISet<string> DeclaredApplicationPolicies { get; set; }
2222
public int? InhibitAnyDepth { get; set; }
23-
public LowLevelList<CertificatePolicyMapping> PolicyMapping { get; set; }
23+
public List<CertificatePolicyMapping> PolicyMapping { get; set; }
2424
public int? InhibitMappingDepth { get; set; }
2525
public int? RequireExplicitPolicyDepth { get; set; }
2626

@@ -356,10 +356,10 @@ private static ISet<string> ReadCertPolicyExtension(X509Extension extension)
356356
return policies;
357357
}
358358

359-
private static LowLevelList<CertificatePolicyMapping> ReadCertPolicyMappingsExtension(X509Extension extension)
359+
private static List<CertificatePolicyMapping> ReadCertPolicyMappingsExtension(X509Extension extension)
360360
{
361361
DerSequenceReader reader = new DerSequenceReader(extension.RawData);
362-
LowLevelList<CertificatePolicyMapping> mappings = new LowLevelList<CertificatePolicyMapping>();
362+
List<CertificatePolicyMapping> mappings = new List<CertificatePolicyMapping>();
363363

364364
while (reader.HasData)
365365
{

‎src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/ChainPal.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ private static void CheckRevocationMode(X509RevocationMode revocationMode)
4343
{
4444
if (revocationMode != X509RevocationMode.NoCheck)
4545
{
46+
// TODO (#2203): Add support for revocation once networking is ready.
4647
throw new NotImplementedException(SR.WorkInProgress);
4748
}
4849
}

‎src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/DerSequenceReader.cs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,9 @@ internal string ReadOidAsString()
106106
// and avoid re-alloc.
107107
StringBuilder builder = new StringBuilder(contentLength * 4);
108108

109+
// The first byte is ((X * 40) + Y), where X is the first segment and Y the second.
110+
// ISO/IEC 8825-1:2003 section 8.19.4
111+
109112
byte firstByte = _data[_position];
110113
byte first = (byte)(firstByte / 40);
111114
byte second = (byte)(firstByte % 40);
@@ -114,6 +117,12 @@ internal string ReadOidAsString()
114117
builder.Append('.');
115118
builder.Append(second);
116119

120+
// For the rest of the segments, the high bit on the byte is a continuation marker,
121+
// and data is loaded into a BigInteger 7 bits at a time.
122+
//
123+
// When the high bit is 0, the segment ends, so emit a '.' between it and the next one.
124+
//
125+
// ISO/IEC 8825-1:2003 section 8.19.2, and the .NET representation of Oid.Value.
117126
bool needDot = true;
118127
BigInteger bigInt = new BigInteger(0);
119128

@@ -167,6 +176,8 @@ internal string ReadIA5String()
167176
EatTag(DerTag.IA5String);
168177
int contentLength = EatLength();
169178

179+
// IA5 (International Alphabet - 5) is functionally equivalent to 7-bit ASCII.
180+
170181
string ia5String = Encoding.ASCII.GetString(_data, _position, contentLength);
171182
_position += contentLength;
172183

‎src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509CertificateReader.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ internal unsafe OpenSslX509CertificateReader(byte[] data)
5656

5757
if (cert.IsInvalid)
5858
{
59-
throw new CryptographicException();
59+
throw new CryptographicException(Interop.libcrypto.GetOpenSslErrorString());
6060
}
6161

6262
// X509_check_purpose has the effect of populating the sha1_hash value,

‎src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ public void Dispose()
2020
{
2121
if (flags != X509VerificationFlags.NoFlag)
2222
{
23+
// TODO (#2204): Add support for X509VerificationFlags, or throw PlatformNotSupportedException.
2324
throw new NotSupportedException(SR.WorkInProgress);
2425
}
2526

@@ -43,7 +44,7 @@ public static IChainPal BuildChain(
4344
DateTime verificationTime)
4445
{
4546
X509ChainElement[] elements;
46-
LowLevelList<X509ChainStatus> overallStatus = new LowLevelList<X509ChainStatus>();
47+
List<X509ChainStatus> overallStatus = new List<X509ChainStatus>();
4748

4849
// An X509_STORE is more comparable to Cryptography.X509Certificate2Collection than to
4950
// Cryptography.X509Store. So read this with OpenSSL eyes, not CAPI/CNG eyes.
@@ -104,7 +105,7 @@ public static IChainPal BuildChain(
104105

105106
for (int i = 0; i < chainSize; i++)
106107
{
107-
LowLevelList<X509ChainStatus> status = new LowLevelList<X509ChainStatus>();
108+
List<X509ChainStatus> status = new List<X509ChainStatus>();
108109

109110
if (i == errorDepth)
110111
{
@@ -173,7 +174,7 @@ public static IChainPal BuildChain(
173174
StatusInformation = SR.Chain_NoPolicyMatch,
174175
};
175176

176-
var elementStatus = new LowLevelList<X509ChainStatus>(leafElement.ChainElementStatus.Length + 1);
177+
var elementStatus = new List<X509ChainStatus>(leafElement.ChainElementStatus.Length + 1);
177178
elementStatus.AddRange(leafElement.ChainElementStatus);
178179

179180
AddUniqueStatus(elementStatus, ref chainStatus);
@@ -193,7 +194,7 @@ public static IChainPal BuildChain(
193194
};
194195
}
195196

196-
private static void AddUniqueStatus(LowLevelList<X509ChainStatus> list, ref X509ChainStatus status)
197+
private static void AddUniqueStatus(IList<X509ChainStatus> list, ref X509ChainStatus status)
197198
{
198199
X509ChainStatusFlags statusCode = status.Status;
199200

@@ -275,11 +276,13 @@ private static X509ChainStatusFlags MapVerifyErrorToChainStatus(Interop.libcrypt
275276
return X509ChainStatusFlags.HasNotSupportedCriticalExtension;
276277

277278
case Interop.libcrypto.X509VerifyStatusCode.X509_V_ERR_CERT_CHAIN_TOO_LONG:
278-
case Interop.libcrypto.X509VerifyStatusCode.X509_V_ERR_OUT_OF_MEM:
279279
throw new CryptographicException();
280280

281+
case Interop.libcrypto.X509VerifyStatusCode.X509_V_ERR_OUT_OF_MEM:
282+
throw new OutOfMemoryException();
283+
281284
default:
282-
Debug.Assert(false, "Unrecognized X509VerifyStatusCode:" + code);
285+
Debug.Fail("Unrecognized X509VerifyStatusCode:" + code);
283286
throw new CryptographicException();
284287
}
285288
}
@@ -301,6 +304,13 @@ internal static X509Certificate2Collection FindCandidates(
301304
X509Certificate2Collection rootCerts = rootStore.Certificates;
302305
X509Certificate2Collection intermediateCerts = intermediateStore.Certificates;
303306

307+
X509Certificate2Collection[] storesToCheck =
308+
{
309+
extraStore,
310+
intermediateCerts,
311+
rootCerts,
312+
};
313+
304314
while (toProcess.Count > 0)
305315
{
306316
X509Certificate2 current = toProcess.Dequeue();
@@ -312,20 +322,15 @@ internal static X509Certificate2Collection FindCandidates(
312322

313323
X509Certificate2Collection results = FindIssuer(
314324
current,
315-
extraStore,
316-
intermediateCerts,
317-
rootCerts);
325+
storesToCheck);
318326

319327
if (results != null)
320328
{
321329
foreach (X509Certificate2 result in results)
322330
{
323-
if (result != null)
331+
if (!candidates.Contains(result))
324332
{
325-
if (!candidates.Contains(result))
326-
{
327-
toProcess.Enqueue(result);
328-
}
333+
toProcess.Enqueue(result);
329334
}
330335
}
331336
}
@@ -337,7 +342,7 @@ internal static X509Certificate2Collection FindCandidates(
337342

338343
private static X509Certificate2Collection FindIssuer(
339344
X509Certificate2 cert,
340-
params X509Certificate2Collection[] stores)
345+
X509Certificate2Collection[] stores)
341346
{
342347
if (StringComparer.Ordinal.Equals(cert.Subject, cert.Issuer))
343348
{

‎src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/StorePal.cs

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,12 @@
22
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
33

44
using System;
5+
using System.Collections.Generic;
6+
using System.Diagnostics;
57
using System.IO;
68
using System.Security.Cryptography;
79
using System.Security.Cryptography.X509Certificates;
10+
using System.Threading;
811

912
namespace Internal.Cryptography.Pal
1013
{
@@ -37,14 +40,20 @@ public static IStorePal FromSystemStore(string storeName, StoreLocation storeLoc
3740
{
3841
if (storeLocation != StoreLocation.LocalMachine)
3942
{
43+
// TODO (#2206): Support CurrentUser persisted stores.
4044
throw new NotImplementedException();
4145
}
4246

4347
if (openFlags.HasFlag(OpenFlags.ReadWrite))
4448
{
49+
// TODO (#2206): Support CurrentUser persisted stores
50+
// (they'd not be very useful without the ability to add/remove content)
4551
throw new NotImplementedException();
4652
}
4753

54+
// The static store approach here is making an optimization based on not
55+
// having write support. Once writing is permitted the stores would need
56+
// to fresh-read whenever being requested (or use FileWatcher/etc).
4857
if (s_machineRootStore == null)
4958
{
5059
lock (s_machineIntermediateStore)
@@ -66,6 +75,7 @@ public static IStorePal FromSystemStore(string storeName, StoreLocation storeLoc
6675
return CloneStore(s_machineIntermediateStore);
6776
}
6877

78+
// TODO (#2207): Support the rest of the stores, or throw PlatformNotSupportedException.
6979
throw new NotImplementedException();
7080
}
7181

@@ -84,6 +94,10 @@ private static IStorePal CloneStore(X509Certificate2Collection seed)
8494

8595
private static void LoadMachineStores()
8696
{
97+
Debug.Assert(
98+
Monitor.IsEntered(s_machineIntermediateStore),
99+
"LoadMachineStores assumes a lock(s_machineIntermediateStore)");
100+
87101
X509Certificate2Collection rootStore = new X509Certificate2Collection();
88102

89103
DirectoryInfo directoryInfo;
@@ -106,6 +120,9 @@ private static void LoadMachineStores()
106120
return;
107121
}
108122

123+
HashSet<X509Certificate2> uniqueRootCerts = new HashSet<X509Certificate2>();
124+
HashSet<X509Certificate2> uniqueIntermediateCerts = new HashSet<X509Certificate2>();
125+
109126
foreach (FileInfo file in directoryInfo.EnumerateFiles())
110127
{
111128
byte[] bytes;
@@ -116,6 +133,18 @@ private static void LoadMachineStores()
116133
}
117134
catch (IOException)
118135
{
136+
// Broken symlink, symlink to a network file share that's timing out,
137+
// file was deleted since being enumerated, etc.
138+
//
139+
// Skip anything that we can't read, we'll just be a bit restrictive
140+
// on our trust model, that's all.
141+
continue;
142+
}
143+
catch (UnauthorizedAccessException)
144+
{
145+
// If, for some reason, one of the files is not world-readable,
146+
// and this user doesn't have access to read it, just pretend it
147+
// isn't there.
119148
continue;
120149
}
121150

@@ -127,19 +156,23 @@ private static void LoadMachineStores()
127156
}
128157
catch (CryptographicException)
129158
{
159+
// The data was in a format we didn't understand. Maybe it was a text file,
160+
// or just a certificate type we don't know how to read. Either way, let's load
161+
// what we can.
130162
continue;
131163
}
132164

165+
// The HashSets are just used for uniqueness filters, they do not survive this method.
133166
if (StringComparer.Ordinal.Equals(cert.Subject, cert.Issuer))
134167
{
135-
if (!rootStore.Contains(cert))
168+
if (uniqueRootCerts.Add(cert))
136169
{
137170
rootStore.Add(cert);
138171
}
139172
}
140173
else
141174
{
142-
if (!s_machineIntermediateStore.Contains(cert))
175+
if (uniqueIntermediateCerts.Add(cert))
143176
{
144177
s_machineIntermediateStore.Add(cert);
145178
}

‎src/System.Security.Cryptography.X509Certificates/src/Resources/Strings.resx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,4 +207,4 @@
207207
<data name="Chain_NoPolicyMatch" xml:space="preserve">
208208
<value>The certificate has invalid policy.</value>
209209
</data>
210-
</root>
210+
</root>

0 commit comments

Comments
 (0)