Skip to content

Conversation

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Apr 4, 2023

Please review a doc update to add @spec into the rest of the files in java.base (compared to those in JDK-8305206 PR #13248)


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8305406: Add @spec tags in java.base/java.* (part 2) (Enhancement - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13336/head:pull/13336
$ git checkout pull/13336

Update a local copy of the PR:
$ git checkout pull/13336
$ git pull https://git.openjdk.org/jdk.git pull/13336/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13336

View PR using the GUI difftool:
$ git pr show -t 13336

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13336.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 4, 2023

👋 Welcome back jjg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 4, 2023
@openjdk
Copy link

openjdk bot commented Apr 4, 2023

@jonathan-gibbons The following labels will be automatically applied to this pull request:

  • net
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@mlbridge
Copy link

mlbridge bot commented Apr 4, 2023

Webrevs

*
* @spec serialization/index.html Java Object Serialization Specification
* @spec https://www.rfc-editor.org/info/rfc5280
* RFC 5280: Internet X.509 Public Key Infrastructure Certificate
Copy link
Member

Choose a reason for hiding this comment

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

any reason why you indented this comment with 8 spaces? All others are indented with 6

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 will change it to be consistent.

@openjdk
Copy link

openjdk bot commented Apr 5, 2023

@jonathan-gibbons This change is no longer ready for integration - check the PR body for details.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 5, 2023
@openjdk openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels Apr 5, 2023
@seanjmullan
Copy link
Member

I plan to review this but may need a couple of days.

@seanjmullan
Copy link
Member

There are references to other specifications missing, like NIST Special Publication 800-90A Revision 1, referenced in java.security.DrbgParameters. I think there are others, I haven't done a thorough review yet. Will there be subsequent reviews for these, or should I point them out and have them resolved as part of this PR?

@jonathan-gibbons
Copy link
Contributor Author

jonathan-gibbons commented Apr 13, 2023

There are references to other specifications missing, like NIST Special Publication 800-90A Revision 1, referenced in java.security.DrbgParameters. I think there are others, I haven't done a thorough review yet. Will there be subsequent reviews for these, or should I point them out and have them resolved as part of this PR?

This should contain all the specs on spec sites that I know about, so if there are other specs that I missed, please indicate that, and we can either handle them here or in follow-up reviews. Your choice.

I see that overall there are 7 href to something in nist.gov: 6 in nvlpubs.nist.gov; 1 in circa.nist.gov

@bradfordwetmore
Copy link
Contributor

I'm coming to this late, but what is the breadth of the specs you're trying to call out? Where did you obtain this list? Are all of these changes coming from existing mentions in the current APIs, and you're just adding a @spec in various places? Or are you trying to be complete, or just list a representative sample? In part 1, I saw you moved some of the spec mentions to be in a @spec, but in this PR, you're adding specs in the APIs.

In many of our APIs, we mention things "...such as...RFC 2246...", but we make no effort to be complete by providing a list of specs.

For example:

SSLEngine.java: only TLSv1.0 was mentioned, but there's also SSLv3/1.1/1.2/1.3, and DTLS 1.0/1.2.

SSLSocket.java: your change only lists 7301, but not 2246. But same issue as SSLEngine, there are others specs this also applies to.

java.security.Key.java: RFC 5280 was the only spec called out. There are many other Key types.

SecureRandom: RFC 4086 was called out. There are others.

If you want to mention a bunch of the security specs, I think we need to better understand the scope of what you're trying to do, and how this will be kept in sync with Chapter 4 of the Security Documentation (Providers): which also could use some updates-e.g. TLSv1.x RFCs, but that is another RFE for another day.

@jonathan-gibbons
Copy link
Contributor Author

@bradfordwetmore

I'm coming to this late, but what is the breadth of the specs you're trying to call out?

This is a mostly automated update based on existing references to well-known specifications.

  • Mostly automated means it was done by a custom utility program, but I hand-edited the output to improve line-wrapping, etc.
  • Existing references means found in <a href=...> in the same doc comment to which the @spec is added, where that HTML link may appear in either narrative text or in a @see tag
  • Well-known specifications means that the utility was looking for well-known hosts/urls. The list of the hosts that was used is ietf.org, unicode.org, w3.org, iana.org, iso.org, and references to the sibling specs directory that exists beside the main api directory. While those are the hosts, the pattern matching was more specific to singleton URLs or groups of URLs at each site.

The update uses "normative"/"preferred" URLs for each spec, to avoid the variety of different URLs used for some of the specs, reflecting different hosts and/or paths for the same specification.
The goal is just to add these @spec tags without any semantic change to the specification. As such, no CSR is required for this work.

If there are additional @spec tags that should be added for existing references, I can do that here in this PR, or in followup work. It has already been noted that I missed the references to nist.gov.

If there are additional specifications that should be included (that are not currently mentioned) that is a task for the relevant developers in the relevant component teams. I would expect any such work to be done separately, later.

It is also a future-goal to clean up the existing href references to the same general form of URL as given in the @spec tag, thus normalizing the host and path that we use for each specification (or the root of a multi-page specification.).

The effect of the @spec tags is to list the specifications for a declaration in a list headed External Specifications, similar to the existing See Also list. Eventually, we will enable a new summary page headed External Specifications that lists all the specs listed in @spec tags, providing links to where the specs are mentioned. That page is currently temporarily disabled until we have a more complete set of @spec tags in most or all modules. Using the canonical form of spec URLs and titles in @spec tags, and having the cross-referenced list on that new External Specifications page should make it easier to find and maintain these references going forward.

@wangweij
Copy link
Contributor

wangweij commented May 5, 2023

We have quite some standard-names.html#anchorName links (Ex:

* "{@docRoot}/../specs/security/standard-names.html#signature-algorithms">
). I don't see any of them here. Is this style allowed in a @spec tag?

I also see no #anchorName for RFC links.

And in this case, shall I also spell out the section name in the text?

@jonathan-gibbons
Copy link
Contributor Author

jonathan-gibbons commented May 5, 2023

We have quite some standard-names.html#anchorName links (Ex:

* "{@docRoot}/../specs/security/standard-names.html#signature-algorithms">

). I don't see any of them here. Is this style allowed in a @spec tag?
I also see no #anchorName for RFC links.

And in this case, shall I also spell out the section name in the text?

  1. You can use a relative URL, relative to the specs directory, so something like:

    @spec security/standard-names.html Standard Names

  2. The intent of the @spec tag is to identify the overall/root URL for each specification, not to identify all the interesting places within that specification.

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 3, 2023

@jonathan-gibbons This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@jonathan-gibbons
Copy link
Contributor Author

keep-alive comment

@jonathan-gibbons
Copy link
Contributor Author

I checked out references to nist.gov.

I found 7 references to 4 documents:

$ grep -r '\*.*href=[^ ]*nist.gov' open/src/java.base | grep -o 'nist.gov[^"]*'
nist.gov/nistpubs/FIPS/NIST.FIPS.140-2.pdf
nist.gov/publications/fips/fips186-3/fips_186-3.pdf
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-90Ar1.pdf
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>

Of these 4, 1 shows a page that says the spec has been superseded, and one is effectively a 404

Oops, that's not standard?!
Sorry, we cannot find that page.

The page you requested cannot be found at this time.

I would like to defer these issues to be handled separately, and add @spec tags for these pages later, once the appropriate specs have been identified.

@jonathan-gibbons
Copy link
Contributor Author

I checked out references to nist.gov.

I found 7 references to 4 documents:

$ grep -r '\*.*href=[^ ]*nist.gov' open/src/java.base | grep -o 'nist.gov[^"]*'
nist.gov/nistpubs/FIPS/NIST.FIPS.140-2.pdf
nist.gov/publications/fips/fips186-3/fips_186-3.pdf
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-90Ar1.pdf
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>

Of these 4, 1 shows a page that says the spec has been superseded, and one is effectively a 404

Oops, that's not standard?!
Sorry, we cannot find that page.

The page you requested cannot be found at this time.

I would like to defer these issues to be handled separately, and add @spec tags for these pages later, once the appropriate specs have been identified.

Update, the apparent 404 is explained by a scripting error, due to an unquoted attribute value.

@ferakocz
Copy link
Contributor

ferakocz commented Jun 9, 2023

I checked out references to nist.gov.
I found 7 references to 4 documents:

$ grep -r '\*.*href=[^ ]*nist.gov' open/src/java.base | grep -o 'nist.gov[^"]*'
nist.gov/nistpubs/FIPS/NIST.FIPS.140-2.pdf
nist.gov/publications/fips/fips186-3/fips_186-3.pdf
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-90Ar1.pdf
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>
nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf>

Of these 4, 1 shows a page that says the spec has been superseded, and one is effectively a 404

Oops, that's not standard?!
Sorry, we cannot find that page.

The page you requested cannot be found at this time.

I would like to defer these issues to be handled separately, and add @spec tags for these pages later, once the appropriate specs have been identified.

Update, the apparent 404 is explained by a scripting error, due to an unquoted attribute value.

The site name in the urls has changed to nvlpubs.nist.gov .

For the DSS, the latest version in which DSA is described is
nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf . It will be with withdrawn on February 23, 2024, though, and the superseding FIPS 186-5 standard says that it can only be used for signature verification from then on, but for the algorithm description it points back to FIPS 186-4.

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 8, 2023

@jonathan-gibbons This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 5, 2023

@jonathan-gibbons This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Sep 5, 2023
@hns
Copy link
Member

hns commented Oct 3, 2024

/open

@openjdk
Copy link

openjdk bot commented Oct 3, 2024

@hns Only the pull request author can set the pull request state to "open"

@openjdk openjdk bot changed the title JDK-8305406: Add @spec tags in java.base/java.* (part 2) 8305406: Add @spec tags in java.base/java.* (part 2) Oct 3, 2024
@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

7 participants