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.

Sign up for GitHub

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

Jump to bottom

[apps/ca] Clean and simplify CA-certificate output path/filename construction #1936

Closed
wants to merge 5 commits into from
Closed

[apps/ca] Clean and simplify CA-certificate output path/filename construction #1936

wants to merge 5 commits into from

Conversation

FdaSilvaYY
Copy link
Contributor

@FdaSilvaYY FdaSilvaYY commented Nov 16, 2016 •
edited

Checklist
  • CLA is signed
Description of change

Follows discussions in PR #1569
( Found a subtle bug while rereading my changes. => was merged as 0db1fb3 )
Applicable to master and 1.1.0 only.

richsalz
richsalz reviewed Nov 16, 2016
View reviewed changes
Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

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

remove the CI changes, otherwise looks good.

@FdaSilvaYY
Copy link
Contributor Author

@richsalz : Yes, i forget to drop this one; I use it as I'm bored of CI mail notifications.
BTW, someone ask a few days ago about this:
https://docs.travis-ci.com/user/customizing-the-build/#Skipping-a-build

@FdaSilvaYY FdaSilvaYY changed the title [Apps/ca] one bugfix and various cleanups [apps/ca] one bugfix and various cleanups Nov 16, 2016
levitte
levitte reviewed Nov 16, 2016
View reviewed changes
apps/ca.c Outdated

j = ASN1_STRING_length(serialNumber);
p = (const char *)ASN1_STRING_get0_data(serialNumber);
new_cert[PATH_MAX - 1] = '\0';
Copy link
Member

Choose a reason for hiding this comment

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

There's really no need for this at all, since the following strcpy will place the NUL character in its correct position anyway.

@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch 2 times, most recently from c655ed5 to 8e22b7e Compare November 23, 2016 20:34
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch 4 times, most recently from 5cfcabb to 7be421c Compare December 6, 2016 22:48
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch 3 times, most recently from 72ae239 to 6acf4b6 Compare December 10, 2016 18:06
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch 3 times, most recently from ad5f9d7 to d36c67e Compare January 10, 2017 19:42
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch 2 times, most recently from 6c5b513 to 4ba3d09 Compare January 21, 2017 22:21
@FdaSilvaYY FdaSilvaYY mentioned this pull request Jan 22, 2017
Closed
1 task
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch from 4ba3d09 to 057f236 Compare January 23, 2017 21:53
@FdaSilvaYY FdaSilvaYY changed the title [apps/ca] one bugfix and various cleanups [apps/ca] Various cleanups Jan 23, 2017
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch 2 times, most recently from c418dd8 to 3c4187f Compare January 31, 2017 21:42
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch 3 times, most recently from 0948201 to ff432cf Compare February 10, 2017 22:17
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch from cb2dcca to 9290342 Compare May 2, 2017 20:14
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch from 9290342 to 61e99bc Compare June 13, 2017 21:07
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch from 61e99bc to 1e23dc7 Compare June 20, 2017 21:59
@FdaSilvaYY
Copy link
Contributor Author

Ping ! old PR that fix some remarks raised in #1569.
It should improve the coverage

richsalz
richsalz approved these changes Jun 21, 2017
View reviewed changes
Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

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

thanks for the nudge!

@richsalz richsalz self-assigned this Jun 21, 2017
@richsalz richsalz added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Jun 21, 2017
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch from cc08e28 to 82efa8d Compare July 28, 2017 23:06
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch from 82efa8d to bff65cc Compare August 26, 2017 14:45
@FdaSilvaYY FdaSilvaYY force-pushed the fix-ca-max-path-limit-checking branch 3 times, most recently from cb452c3 to c16ac24 Compare September 22, 2017 22:10
@FdaSilvaYY FdaSilvaYY changed the title [code health?] [apps/ca] Clean and simplify CA-certificate output path/filename construction [apps/ca] Clean and simplify CA-certificate output path/filename construction Oct 2, 2017
@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 5, 2017
@FdaSilvaYY
Copy link
Contributor Author

Thanks !!

@FdaSilvaYY
Copy link
Contributor Author

Ping !
After two Approvals, it's ready for push, no ? ;)

levitte pushed a commit that referenced this pull request Oct 16, 2017
@FdaSilvaYY
Some cleanups to apps/ca.c
b589581 
Few code format fixup
Fix limit computation; was too strict by 2 bytes.
Simplify computation of buffer limits
Checking is strictly same as sizeof(".pem") == 5
Simplify loop of code for certificate filename creation
Fix MAX_PATH usage

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #1936)
@richsalz
Copy link
Contributor

Squashed and merged. Thank you for the cleanups!

@richsalz richsalz closed this Oct 16, 2017
@FdaSilvaYY FdaSilvaYY deleted the fix-ca-max-path-limit-checking branch November 1, 2017 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte left review comments

@richsalz richsalz richsalz approved these changes

@paulidale paulidale paulidale approved these changes

Assignees

@richsalz richsalz

Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@FdaSilvaYY @richsalz @levitte @paulidale