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

CT documentation #1372

Closed
wants to merge 25 commits into from
Closed

CT documentation #1372

wants to merge 25 commits into from

Conversation

RJPercival
Copy link
Contributor

POD documentation for the Certificate Transparency API - fixes #1274.

This is a draft, and is likely to be missing useful information - please point out what would be helpful to add. Some of the PODs are missing most of their content - I'll update this PR over the next couple of days filling those holes.

Therefore, it is useful to be able to look up more information about a log
(e.g. its public key) using this LogID.

B<CTLOG_STORE_get0_log_by_id>() provides a way to do this. It will find a CTLOG
Copy link
Member

Choose a reason for hiding this comment

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

Ping @richsalz - I think our style is to not put function names like this in B<> - am I right? We should have a documentation style guide.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, but it's actually a POD style. Any foo() (closing parenthesis mandatory) will automatically be emphasised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes we should have a style guide but you are right, function names with closing paren's do not go inside a B markup.
Tip: run ./util/find-doc-nits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip, I wasn't aware of that tool. I've resolved all of the issues it highlighted.

RJPercival pushed a commit to RJPercival/openssl that referenced this pull request Aug 5, 2016
Changes them to have clearer ownership semantics, as suggested in
openssl#1372 (comment).
@mattcaswell
Copy link
Member

Looking better. There are still a few outstanding comments, and still some pods that need filling in. I assume you are planning to fill these in?

levitte pushed a commit that referenced this pull request Aug 15, 2016
Changes them to have clearer ownership semantics, as suggested in
#1372 (comment).

Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #1408)
@mattcaswell
Copy link
Member

@RJPercival what is the status of this...9 days to 1.1.0 release...?

@RJPercival
Copy link
Contributor Author

I'm working on it again today, and will have it done this week. I was OOO
all of last week.

On Tue, 16 Aug 2016 at 10:30 mattcaswell notifications@github.com wrote:

@RJPercival https://github.com/RJPercival what is the status of
this...9 days to 1.1.0 release...?


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1372 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB29lNSly-xPLlhxS_9ijyTtRUOL6UTZks5qgYNEgaJpZM4JZ6fX
.

@mattcaswell
Copy link
Member

3 days to go...

Rob Percival added 2 commits August 23, 2016 18:15
SCT_verify_v1 has been removed and SCT_verify is no longer part of the
public API.
@richsalz
Copy link
Contributor

I re-read every single comment here. I think this is the only outstanding change:

diff --git a/doc/crypto/SCT_new.pod b/doc/crypto/SCT_new.pod
index a12408b..754df23 100644
--- a/doc/crypto/SCT_new.pod
+++ b/doc/crypto/SCT_new.pod
@@ -145,7 +145,7 @@ required for verifying the SCT.
 Some of the setters return int, instead of void. These will all return 1 on
 success, 0 on failure. They will not make changes on failure.

-Most of the setters will reset the validation status of the SCT to
+All setters will reset the validation status of the SCT to
 SCT_VALIDATION_STATUS_NOT_SET (see L<SCT_verify(3)>).

 SCT_set_source() will call SCT_set_log_entry_type() if the type of
;

@RJPercival
Copy link
Contributor Author

Fixed, along with PR #1487.

Ownership semantics and function names have changed.
@mattcaswell
Copy link
Member

Is this now finished (aside from review?). With a quick scan through I didn't spot any sections still to be filled in.

@richsalz
Copy link
Contributor

All issues raised so far have been addressed. Plus-one from me. I encourage a separate MR for new podpages.

@richsalz
Copy link
Contributor

+1 merge

@richsalz
Copy link
Contributor

any new doc updates or additions should go into a separate MR.

each SCT (if that log is in the CTLOG_STORE). Alternatively, NULL can be passed
as the CTLOG_STORE parameter to disable this feature.

B<SCT_validation_status_string> will return the validation status of an SCT as
Copy link
Member

Choose a reason for hiding this comment

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

Drop the "B" tag and just write SCT_validation_status_string()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@RJPercival
Copy link
Contributor Author

All comments addressed.

@mattcaswell
Copy link
Member

+1 subject to the following commit being added. If I can get a +1 from the team for that I can push this:

From 43acc3254a04a5a1fc19de45f6524a298beca25d Mon Sep 17 00:00:00 2001
From: Matt Caswell <matt@openssl.org>
Date: Wed, 24 Aug 2016 10:03:04 +0100
Subject: [PATCH] Misc tweaks and fixes for the CT documentation

---
 doc/crypto/CTLOG_new.pod    | 1 -
 doc/crypto/SCT_print.pod    | 4 ++--
 doc/crypto/ct.pod           | 2 +-
 doc/crypto/o2i_SCT_LIST.pod | 6 +++---
 4 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/doc/crypto/CTLOG_new.pod b/doc/crypto/CTLOG_new.pod
index de196f4..ccda6b9 100644
--- a/doc/crypto/CTLOG_new.pod
+++ b/doc/crypto/CTLOG_new.pod
@@ -45,7 +45,6 @@ the string remains with the CTLOG.

 CTLOG_get0_public_key() returns the public key of the CT log. Ownership of the
 EVP_PKEY remains with the CTLOG.
-with

 =head1 RETURN VALUES

diff --git a/doc/crypto/SCT_print.pod b/doc/crypto/SCT_print.pod
index d14a98e..88ad43e 100644
--- a/doc/crypto/SCT_print.pod
+++ b/doc/crypto/SCT_print.pod
@@ -25,8 +25,8 @@ is provided, it will be used to print the description of the CT log that issued
 each SCT (if that log is in the CTLOG_STORE). Alternatively, NULL can be passed
 as the CTLOG_STORE parameter to disable this feature.

-B<SCT_validation_status_string> will return the validation status of an SCT as
-a human-readable string. Call L<SCT_validate>() or SCT_LIST_validate()
+SCT_validation_status_string() will return the validation status of an SCT as
+a human-readable string. Call SCT_validate() or SCT_LIST_validate()
 beforehand in order to set the validation status of an SCT first.

 =head1 SEE ALSO
diff --git a/doc/crypto/ct.pod b/doc/crypto/ct.pod
index 65a4ea2..bdcda98 100644
--- a/doc/crypto/ct.pod
+++ b/doc/crypto/ct.pod
@@ -31,7 +31,7 @@ functions for:

 L<d2i_SCT_LIST(3)>,
 L<CTLOG_STORE_new(3)>,
-L<CTLOG_STORE_get0_log_by_id(3),
+L<CTLOG_STORE_get0_log_by_id(3)>,
 L<SCT_new(3)>,
 L<SCT_print(3)>,
 L<SCT_verify(3)>,
diff --git a/doc/crypto/o2i_SCT_LIST.pod b/doc/crypto/o2i_SCT_LIST.pod
index 302a288..82922fc 100644
--- a/doc/crypto/o2i_SCT_LIST.pod
+++ b/doc/crypto/o2i_SCT_LIST.pod
@@ -23,14 +23,14 @@ treated and the return values.

 =head1 RETURN VALUES

-All of the functions have return values consist with those stated for
+All of the functions have return values consistent with those stated for
 L<d2i_SCT_LIST> and L<i2d_SCT_LIST>.

 =head1 SEE ALSO

 L<ct(3)>,
-L(d2i_SCT_LIST(3)>,
-L(i2d_SCT_LIST(3)>
+L<d2i_SCT_LIST(3)>,
+L<i2d_SCT_LIST(3)>

 =head1 HISTORY

-- 
2.7.4

@mattcaswell mattcaswell added reviewed approval: done This pull request has the required number of approvals and removed approval: done This pull request has the required number of approvals labels Aug 24, 2016
@mattcaswell
Copy link
Member

Oh...scratch that last comment. Your updates didn't show up here so didn't see them.

@mattcaswell
Copy link
Member

+1

@mattcaswell
Copy link
Member

Ping @richsalz for reconfirm

@richsalz
Copy link
Contributor

+1 from me

@mattcaswell
Copy link
Member

Merged. Thanks!

levitte pushed a commit that referenced this pull request Aug 24, 2016
The prior wording was less accurate.
See #1372 (comment).

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing CT documentation
5 participants