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

Add security design docs for Dictionary, HashCode, and StringComparer #108864

Merged
merged 7 commits into from
Oct 17, 2024

Conversation

GrabYourPitchforks
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks commented Oct 15, 2024

Publishing the internal security design docs for System.Collections.Generic.Dictionary, System.Collections.Generic.HashSet, System.HashCode, and System.StringComparer. These have been reviewed by the various subject matter experts, Crypto Board, MSVR, and the security LT.

Note to readers - these are hefty docs. Highly recommend using the "preview" feature instead of looking at the raw markdown.

I'm open to suggestions on where these docs should actually live. Putting them in the existing docs/security directory seemed like an obvious choice. :)

@GrabYourPitchforks GrabYourPitchforks added documentation Documentation bug or enhancement, does not impact product or test code area-Meta labels Oct 15, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

@GrabYourPitchforks
Copy link
Member Author

Remaining markdownlint failure is addressed by #108866

@GrabYourPitchforks
Copy link
Member Author

Going to leave this open for another day or so just in case somebody has feedback about location, linking, etc.

@jkotas
Copy link
Member

jkotas commented Oct 15, 2024

I'm open to suggestions on where these docs should actually live.

If we want these docs to be customer facing, they should live in https://github.com/dotnet/docs and be published at https://learn.microsoft.com/ .

The repo docs are not meant to be customer facing. They are meant for people working in the repo or are curious about internals.

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Oct 15, 2024

If we want these docs to be customer facing ...

The pattern we established during STJ development was that the threat model doc would live in the repo alongside the implementation, which also allows the doc to be versioned alongside the component. Portions of the doc can be excerpted into the relevant API pages on MSDN. Or, in the case of STJ, the customer-facing docs just straight-up link to the threat model. See for example https://learn.microsoft.com/dotnet/standard/serialization/system-text-json/overview#security-information.

During STJ development, we considered putting these docs in a separate repo - either the existing docs repo or a standalone security-docs repo. But the desire to keep everything consolidated in a single versionable place won out. And like with the STJ threat models, these docs are not entirely customer facing. The audience for these docs is spread across maintainers and consumers.

Do you think we should reconsider the decision of where such docs live?

@jkotas
Copy link
Member

jkotas commented Oct 15, 2024

I understand that linking from official docs into dotnet/runtime docs is convenient at first. We have done it here and there. It tends to blur the boundary between the public surface and internal implementation details and cause confusion in the long run. For example, #107341 (comment) is an active PR that is trying to fix one of those confusions.

As added benefit, docs that live in the docs repo get a review from our docs team to ensures consistent customer facing style.

Do you think we should reconsider the decision of where such docs live?

I do not have a strong opinion - as long as you are prepared to deal with fallout caused by linking from official docs into dotnet/runtime docs.

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Oct 15, 2024

Received some feedback that the dictionary doc deserves more discussion of the existing "rehash after 100 collisions" behavior, its limitations, and potential future extensibility. Will send a new iteration in a few hours.

@blowdart
Copy link
Contributor

To put in my penny's worth opinion I like having them alongside the code. They're not every day docs, they don't belong in the docs repo IMO.

@SteveDunn
Copy link
Contributor

Received some feedback that the dictionary doc deserves more discussion of the existing "rehash after 100 collisions" behavior, its limitations, and potential future extensibility. Will send a new iteration in a few hours.

Rehash with the next largest bucket size?

@SteveDunn
Copy link
Contributor

To put in my penny's worth opinion I like having them alongside the code. They're not every day docs, they don't belong in the docs repo IMO.

Agree. Most users just expect security out of the box. For users where security takes higher priority, then digging a bit deeper is required. I think having them as every day docs will worry people (every day consumers) unnecessarily.

@terrajobst
Copy link
Member

In general, here is how I think about the various locations for docs:

  1. Use dotnet/designs for proposals for new potential directions of .NET.
  2. Use dotnet/docs for telling customers how to use .NET.
  3. Use dotnet/runtime for documenting the implementation of .NET.

Threat models are somewhere between (2) and (3). But I agree with @jkotas that using docs has various benefits. By us publishing those docs we primarily intent to inform customers (rather than reminding ourselves how our stuff works). My preference would be dotnet/docs for that reason.

@GrabYourPitchforks
Copy link
Member Author

I like Immo's classification system and how it dictates where markdown files go. Agree that threat model docs fall somewhere between (2) and (3), but I think it's closer to a 10% / 90% split. That is, threat model docs are very technical docs which describe the implementation of the component, how the implementation tries to fulfill various public contracts, and what constraints are placed on maintainers.

While consumers could absolutely benefit from such docs - and the docs in this PR do indeed have sections describing proper usage - a threat model doc is not the appropriate way to present that data to a broad audience. I instead see these technical docs as informing what the user-facing docs should say. The user-facing docs would be very concise and written in a much more approachable language. Perhaps it would merit just a few sentences under the API's Remarks section and an accompanying link back to the real threat model docs as the authoritative source of detailed information.

@GrabYourPitchforks GrabYourPitchforks merged commit 2163b41 into dotnet:main Oct 17, 2024
13 checks passed
@GrabYourPitchforks
Copy link
Member Author

I'll get PRs up shortly in the docs repo for better customer-facing guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Meta documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants