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

Allow X509Chain callers to prevent retrieval of missing certificates #37485

Closed
bartonjs opened this issue Jun 5, 2020 · 8 comments · Fixed by #37680
Closed

Allow X509Chain callers to prevent retrieval of missing certificates #37485

bartonjs opened this issue Jun 5, 2020 · 8 comments · Fixed by #37680
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@bartonjs
Copy link
Member

bartonjs commented Jun 5, 2020

Background and Motivation

Callers sometimes want to build a "fully offline" X509Chain, but the only exposed online/offline mode pertains to revocation processing. Alternatively, they may want to be in a "limited online" state, where the only network operations are for retrieving up-to-date CRL/OCSP responses for the end-entity certificate.

Proposed API

namespace System.Security.Cryptography.X509Certificates
{
    public partial class X509ChainPolicy
    {
+        // false (default) for the current behavior,
+        // true to only use the system stores and the ExtraStore and CustomTrust collections.
+        public bool DisableCertificateFetch { get; set; }
    }
}
@bartonjs bartonjs added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security blocking Marks issues that we want to fast track in order to unblock other important work labels Jun 5, 2020
@bartonjs bartonjs added this to the 5.0 milestone Jun 5, 2020
@ghost
Copy link

ghost commented Jun 5, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jun 5, 2020
@bartonjs bartonjs added api-ready-for-review and removed untriaged New issue has not been triaged by the area owner api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 5, 2020
@stephentoub
Copy link
Member

Would there be any benefit in an enum with all the various places it could get the cert from, rather than a bool that splits them into two categories?

@bartonjs
Copy link
Member Author

bartonjs commented Jun 5, 2020

If we had a fully managed chain processor on all OSes, sure. But for Windows and macOS we only get a boolean for suppressing certificate downloads. Persistent storage (especially for system root trust) can't really be disabled.

@vcsjones
Copy link
Member

vcsjones commented Jun 5, 2020

@bartonjs I assume the intent of this flag is to disable AIA fetching, or anything that can cause the network to be hit (aside from revocation checking)?

Put another way, on Windows, will this set CERT_CHAIN_DISABLE_AUTH_ROOT_AUTO_UPDATE and CERT_CHAIN_DISABLE_AIA? Both? One?

@bartonjs
Copy link
Member Author

bartonjs commented Jun 5, 2020

I had only planned on CERT_CHAIN_DISABLE_AIA. CERT_CHAIN_DISABLE_AUTH_ROOT_AUTO_UPDATE is also reasonable; though I feel like that usually doesn't involve a network. But it might, so why not? 😄.

@vcsjones
Copy link
Member

vcsjones commented Jun 5, 2020

though I feel like that usually doesn't involve a network

Isn't this the flag that controls whether or not the cert chain builder will kick off a "try to get intermediate certificates and new roots from Windows Update"?

@bartonjs
Copy link
Member Author

bartonjs commented Jun 5, 2020

Yeah, but based on some black-box (e.g. yank the network cable) observation 10-15 years ago (could be bad recollection, could have changed since then) I think that the WU bundle ends up just being a background-downloaded bundle and the flag is "ugh, if I need to, crack open the bundle and see if there's anything useful in it". It might have a "I downloaded this too long ago, let's ask live", but there definitely seemed to be a cache, and the side effects that entail from this path being hit.

::shrug::

@bartonjs
Copy link
Member Author

bartonjs commented Jun 5, 2020

Looks good, but we changed the name:

namespace System.Security.Cryptography.X509Certificates
{
    public partial class X509ChainPolicy
    {
+        // false (default) for the current behavior,
+        // true to only use the system stores and the ExtraStore and CustomTrust collections.
+        public bool DisableCertificateDownloads { get; set; }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review blocking Marks issues that we want to fast track in order to unblock other important work labels Jun 5, 2020
@bartonjs bartonjs self-assigned this Jun 5, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
@bartonjs bartonjs removed their assignment Jul 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants