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

Draft to extract release json #9210

Closed
wants to merge 13 commits into from

Conversation

Falco20019
Copy link
Contributor

Resolves #9205

I created a draft as it might be easier to discuss on.

/CC @richlander @leecow

Copy link
Contributor Author

@Falco20019 Falco20019 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some additional thoughts. Bare in mind that 8.0/releases.json and 9.0/releases.json are kept untouched to avoid breaking changes for consumers.

"release-date": "2023-11-14",
"release-version": "8.0.0",
"security": true,
"cve.json": "https://dotnetcli.blob.core.windows.net/dotnet/release-metadata/8.0/8.0.0/cve.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the cve-list from here as it's moved out into cve.json. The security flag is redundant, as cve.json is only currently set when not empty.

It's also possible to always define a cve.json and link it (even when empty) and keep the security flag also either way. For the 8.0/release.json, we would need to keep security there either way for backwards compatibility. For the version-specific and the channel-specific new files, we are free to choose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like bools. They work so well with programming language syntax Everything else has worse ergonomics.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should continue to include the CVE information in this file. It means near zero cost for people to move to this new format. It also means we don't have to change the code that generates this file.

I see the cve.json file as a benefit for people who only want that information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So keeping the list in and linking to it so that people interested would know how to find it? Would be fine with me as the list isn't that big. I thought that it might get bigger once we add more information to each CVE. But even then, it's usually not more then 2-5 CVEs in a single release.

The 8.0/8.0.0/release.json is one of the new files, so I don't fully understand the part about keeping the generator the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we want to only have a index file for all security fixes (similar to the cve.md) or if a full list would be prefered. I personally like to avoid duplication and would prefer the index file. As non-security releases are missing, it's also easier to work with.

{
"release-date": "2023-02-21",
"release-version": "8.0.0-preview.1",
"security": false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non-security releases, I left the cve.json link away. That makes this also implicit. Open for discussion if excplicity is prefered here.

release-notes/8.0/releases-index.json Show resolved Hide resolved
release-notes/8.0/releases-index.json Show resolved Hide resolved
release-notes/8.0/releases-index.json Show resolved Hide resolved
release-notes/releases-index.json Outdated Show resolved Hide resolved
release-notes/9.0/cve-index.json Outdated Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting with .NET 9, I would also love to keep the version folders in preview (like 9.0/preview/preview1) to gather all files (Release Notes, Markdown, API-Diffs, CVEs, ...) together.

The 8.0/preview folder is sadly a bit cluttered to avoid breaking anyone. We COULD add the folders there to store only the *.json files without moving the *.md and api-diff files. But it just felt inconsistent having some in the root and some in sub-folders.

@@ -0,0 +1,16 @@
{
Copy link
Member

@richlander richlander Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit bare. I think we should include a date and .NET version.

Many folks will probably follow the links to this file and then use this content for the actual deserialization step and want a bit more data than just the CVEs. Certainly, there are other ways to do it, but this approach could be very convenient.

Copy link
Contributor Author

@Falco20019 Falco20019 Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, only the existing data so far. It's one of the points that I wanted to discuss what data could make sense in here. Since we want to generate those, the data this would be generated from would (in my view) also have more information, as this is duplicated per version it was fixed in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, "duplication" isn't really the right metric to use. People are going to deserialize these objects. What data will they want? Imagine adding those objects to a dictionary and/or using LINQ on them. Having a bit more information would make that flow super nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the date and .NET version to them. I think we still have an open discussion on what other information might be useful. In my opinion, having the affected version in there might be valuable. There is also the discussion about having a monthly set of those centrally (over all channels) that's not in the draft yet as I wanted to wait for what data we want to have.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a date in the front matter. If you follow the links from cve-index.json, there are no dates. You'd have to also read releases-index.json to get the dates. I think that's the only addition we need for CVEs.

I don't think we need "also applies to" unless we get asked for that. That is calculatable by looking at all the cve.json files for that date for supported versions.

@leecow
Copy link
Member

leecow commented Mar 4, 2024

Adding @joeloff to the conversation as he is a key "customer" for this data.

@joeloff
Copy link
Member

joeloff commented Mar 4, 2024

Is the plan to just make smaller files or to actually publish a bunch of tiny files and change the format?

Read the issue. It will definitely break this, and probably a bunch of external 3rd parties that we know rely on the current format. We also rely on this information in the SDK (through the library I linked).

I can make an argument for why we want to evaluate all the release data. Splitting it up would make the flipside of this change more costly - send additiona requests to get all the files and pieces of information vs. grabbing the index and then grabbing the specific M.N releases JSON.

@richlander
Copy link
Member

richlander commented Mar 5, 2024

We're not going to break the format. We'll have both the big file and a new small (one per release) alternative. So, additive. Works?

@joeloff
Copy link
Member

joeloff commented Mar 5, 2024

We're not going to break the format. We'll have both the big file and a new small (one per release) alternative. So, additive. Works?

Sounds good, thank you for clarifying.

@richlander
Copy link
Member

Not a problem. The initial conversation was confusing.

release-notes/9.0/cve-index.json Outdated Show resolved Hide resolved
release-notes/cve-index.json Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional (possibly interesting data):

  • Affected repositories
  • Affected OS (+ runtime?)
  • Affected versions

Copy link
Contributor Author

@Falco20019 Falco20019 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richlander @leecow Maybe you could give me some input on those two topics.

{
"channel-version": "7.0",
"last-updated": "2024-02-28",
"glibc": [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to make sense from https://github.com/Falco20019/core/blob/extract-release-json/release-notes/7.0/supported-os.md#libc-compatibility but am not completely sure I got it right.

  • CentOS 7 only support x64, so that sounds plausible
  • Ubuntu 18.04 supports x64, arm64andarm, but only armis listed forUbuntu 18.04whereasarm64is the only listed forUbuntu 16.04`
  • arm64 seems to have a higher min-version for .NET 7.0.3 and earlier (which OS does this affect?)
  • Alpine 3.15 only supports x64 and arm64 through musl, but thre is also arm support listed above

@richlander Is glibc and musl required version tightly linked to the OS and architecture and is the OS cycle the min version including all following if not defined otherwise? In that case, I would rename cycle to min-cycle or change cycle to an array like with supported-cycles.

Copy link
Member

@richlander richlander Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think musl vs glibc makes sense as a top-level notion. I don't think it makes sense to list the OSes we use in our build system in this file. That's an implementation detail. The contract is the min versions that result from those choices. That does make sense to list, same as we do in the md file.

I think a libc property could make sense for Linux distros, however, it doesn't add much since we currently only support one muslc distro and I don't see that changing soon.

For .NET 6 and 7, the glibc support differed for Arm64 and x64. That changed with .NET 8+, where we were able to unify the two. They should stay unified going forward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make a proposal on this.

Copy link
Member

@richlander richlander Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal for that section. I skipped the OS content for brevity. Ideally, the OS content would come first. Most readers won't care/need this information and serializers can (at least in theory) skip it.

{
  "channel-version": "6.0",
  "last-updated": "2024-02-28",
  "glibc": [
    {
      "architecture" : "x64",
      "version": "2.17",
      "source": "CentOS 7"
    },
    {
      "architecture" : "arm64",
      "version": "2.23",
      "source": "Ubuntu 16.04"
    },
    {
      "architecture" : "arm32",
      "version": "2.23",
      "source": "Ubuntu 16.04"
    }
  ],
  "musl" : [
    {
      "architecture" : "x64",
      "version": "1.2.2",
      "source": "Alpine 3.13"
    },
    {
      "architecture" : "arm64",
      "version": "1.2.2",
      "source": "Alpine 3.13"
    },
    {
      "architecture" : "arm32",
      "version": "1.2.2",
      "source": "Alpine 3.13"
    }
  ]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using strings as source might make it harder to work with in a parsable form (compared to having it split up into os-name and os-cycle). I also find it strange to duplicate full items in the musl case, as it might introduce the need for users to compare and join those manually.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strings are solely there to enable printing the following text.

image

https://github.com/dotnet/core/blob/main/release-notes/8.0/supported-os.md#libc-compatibility

I don't have another use case in mind where the "source" values would be needed. Do you? There is no user action to take as a result of that information (other than not running .NET 6 on Alpine 3.12, for example).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplication is unfortunate, however, I'm not sure how to solve that w/o adding more complexity to the format. This is effectively 1NF in database schema. It works. As demonstrated, the glibc builds require this approach. It' possible that we'll never need this per-architecture description again, but that's not a great bet to make.

Copy link
Member

@richlander richlander Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do this:

{
  "channel-version": "6.0",
  "last-updated": "2024-02-28",
  "glibc": [
    {
      "architecture" : "x64",
      "version": "2.17",
      "source": "CentOS 7"
    },
    {
      "architecture" : "arm64",
      "version": "2.23",
      "source": "Ubuntu 16.04"
    },
    {
      "architecture" : "arm32",
      "version": "2.23",
      "source": "Ubuntu 16.04"
    }
  ],
  "musl" : [
    {
      "architectures": ["arm32", "arm64", "x64"],
      "version": "1.2.2",
      "source": "Alpine 3.13"
    }
  ]
}

{
"channel-version": "8.0",
"last-updated": "2024-02-28",
"glibc": [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/Falco20019/core/blob/extract-release-json/release-notes/8.0/supported-os.md#libc-compatibility has a lot shorter list than .NET 7 and is also dropping the architectures. Does that mean that there is no limitation or does it mean "the same architectures as above"?

{
"cycle": "7",
"architectures": ["x64"],
"required-version": "2.17"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer min-version over required-version since that more accurately articulates the contract. We could equally just say version and document somewhere that this is the version we target. That's probably the nicest.

Copy link
Member

@richlander richlander Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have a C++ standard library dependency. We should add that, too. I don't think we've ever documented that since it doesn't come up much. If you are able to satisfy libc, then the standard C++ library will probably be satisfied, too. However, being transparent for both is a good plan (now that we're rethinking all of this).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like min-version as it's more expressive then version. Do you have an example for libc? I think I didn't see it in the supported-os.md.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here: #9210 (comment)

In that case, writing min-version and source seems odd.

},
{
"name": "iOS",
"lifecycle-policy": "https://support.apple.com/ios",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this maybe point to an aka.ms? In the unlikely even that URL ever changes, then we only have to update one address, e.g. aka.ms/dotnet-ios-support

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not.

  • Transparent links are greatly preferred. The opaque links are effectively proprietary.
  • It's not our responsibility to prevent broken links for third parties. If broken links appear, folks can propose fixes.
  • Let's keep this "system" as lean as possible.

"last-updated": "2023-02-28"
},
{
"name": "iOS",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this include Window OS versions too? This seems more like a distribution/flavor for some things we do support. If I were to query this information to find out if my OS/distro/version was supported it will likely be "no". We've had requirements back in 1.x and 2.x that required certain patches on certain OS SKUs like 2K8R2, do we really only care about glibc? arm64 on Windows requires a specific version to work, especially emulation, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is a PoC for the format. However, good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly used iOS as an example since it had the edge-case of dropping architectures between releases. Once we have a feeling that the format is somewhat OK, I can play it through for .NET 7-9 for all OS to give a full picture. But since those files will be generated later on, it's more about giving an impression as PoC.

@@ -11,7 +11,9 @@
"product": ".NET",
"release-type" : "sts",
"support-phase": "preview",
"releases.json": "https://dotnetcli.blob.core.windows.net/dotnet/release-metadata/9.0/releases.json"
"releases.json": "https://dotnetcli.blob.core.windows.net/dotnet/release-metadata/9.0/releases.json",
Copy link
Member

@richlander richlander Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we messed up with the format here, some time ago. IMO, we should have two link types:

  • Links to JSON files on super locked down servers (intended for prod scenarios)
  • Links to JSON file on GH where source pedigree can be fully understood

Now is a good time to rationalize that. Of course, no breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand you correctly, we should use GH links here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that we should offer both. We should continue using the Azure links to avoid a compat break, but I think the GitHub links would be a good idea. It's very odd that the files are hosted on GH, but it is impossible to get to those files (with the links provided) if you start the release-index.json.

"cve-id": "CVE-2023-36558",
"cve-url": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-36558"
},
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this:

    {
      "cve-id": "CVE-2023-36038",
      "cve-url": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-36038",
       "ms-url": "https://github.com/dotnet/announcements/issues/286",
       "affected-os" : ["windows"],
       "affected-repos": [
         {
              "repo" : "https://github.com/dotnet/aspnetcore"
         }
       ]
    }

The affected-repo property seems overkill. We should preserve the ability to add more information about repos in the future.

We could do something similar with operating system so that we could add architecture. I don't think that's worth it. There will always be some aspect we cannot represent. For example, this CVE only applies to IIS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a similar concept like "affected-components" or "affected-products", so then it'd be "aspnetcore-runtime", "windowsdesktop", "sdk"? Trying to articulate something (badly) that's like a distributable unit that's more fine-grained than (or untied to) a specific GitHub repo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great point.

    {
      "cve-id": "CVE-2023-36038",
      "cve-url": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-36038",
       "ms-url": "https://github.com/dotnet/announcements/issues/286",
       "affected-os" : ["windows"],
       "affected-components": [
         {
              "component": "Microsoft.AspNetCore.App",
              "versions" : "<8.0.0",
              "repo" : "https://github.com/dotnet/aspnetcore"
         }
       ]
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider updating release.json format to add another level of indexing
5 participants