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

ESSCertIDv2 Update for RFC 3161 (ts module) #771

Closed
wants to merge 12 commits into from

Conversation

Projects
None yet
9 participants
@kleinmrk
Copy link
Contributor

commented Mar 1, 2016

This patch introduces new .conf variables (ess_cert_id_v2 and ess_cert_id_v2_alg) and adds support for ESSCertIDv2 to ts module as defined in RFC5816, thus it removes another hardcoded SHA-1 usage from ts module.

Original behavior (ESSCertID) is preserved.

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2016

Do we need both? Just asking because I don't know anything about TS.

@kleinmrk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2016

Yes, we do need both since it is written in RFC5816 that the certificate identifier is either ESSCertID or ESSCertIDv2.

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2016

Thanks for the clarification.

@@ -88,6 +88,8 @@
#define ENV_CLOCK_PRECISION_DIGITS "clock_precision_digits"
#define ENV_VALUE_YES "yes"
#define ENV_VALUE_NO "no"
#define ENV_ESS_CERT_ID_V2 "ess_cert_id_v2"

This comment has been minimized.

Copy link
@richsalz

richsalz Mar 2, 2016

Contributor

do we really need these define's?

This comment has been minimized.

Copy link
@kleinmrk

kleinmrk Mar 3, 2016

Author Contributor

I have simplified the logic and now only one .conf variable is needed.

const EVP_MD *cert_md = NULL;
const char *md = NULL;

md = NCONF_get_string(conf, section, ENV_ESS_CERT_ID_V2_ALG);

This comment has been minimized.

Copy link
@richsalz

richsalz Mar 2, 2016

Contributor

merge this up onto line 535

goto err;
if (ctx->flags & TS_ESS_CERT_ID_V2) {
certs = ctx->flags & TS_ESS_CERT_ID_CHAIN ? ctx->certs : NULL;
if (!

This comment has been minimized.

Copy link
@richsalz

richsalz Mar 2, 2016

Contributor

break this up into two lines. sc2 = ... and then if (!sc1 == NULL)
dont use ! for pointer tests any more.

goto err;
for (i = 0; i < sk_X509_num(certs); ++i) {
X509 *cert = sk_X509_value(certs, i);
if ((cid = ESS_CERT_ID_V2_new_init(hash_alg, cert, 1)) == NULL

This comment has been minimized.

Copy link
@richsalz

richsalz Mar 2, 2016

Contributor

blank line after var decls.

unsigned int hash_len = EVP_MAX_MD_SIZE;
X509_ALGOR *alg = NULL;

memset(hash, 0, EVP_MAX_MD_SIZE);

This comment has been minimized.

Copy link
@richsalz

richsalz Mar 2, 2016

Contributor

make the last arg be "sizeof(hash)"

unsigned char *p, *pp = NULL;
int len;

len = i2d_ESS_SIGNING_CERT_V2(sc, NULL);

This comment has been minimized.

Copy link
@richsalz

richsalz Mar 2, 2016

Contributor

merge this up to the "int len" line above. keep the blank line :)

ASN1_TYPE *attr;
const unsigned char *p;
attr = PKCS7_get_signed_attribute(si, NID_id_smime_aa_signingCertificateV2);
if (!attr)

This comment has been minimized.

Copy link
@richsalz

richsalz Mar 2, 2016

Contributor

use == NULL as the test. and put a blank line before.

for (i = 0; i < sk_ESS_CERT_ID_V2_num(cert_ids); ++i) {
ESS_CERT_ID_V2 *cid = sk_ESS_CERT_ID_V2_value(cert_ids, i);

const EVP_MD *md;

This comment has been minimized.

Copy link
@richsalz

richsalz Mar 2, 2016

Contributor

swap this and the blank line above :)

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2016

a few minor style issues. I didn't always comment on all of them, so please make sure to check that there's a blank line after variable declarations and don't use ! for pointer checking.
rebase, squash, repost and then I can plus-one this.

@kleinmrk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2016

@richsalz thank you for the review.
What do you mean by repost? Should I open a new pull request?

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2016

No, this MR is fine. Just go to the branch, rebase on mater and push it to github again (-f flag)

@kleinmrk kleinmrk force-pushed the kleinmrk:cert_id_v2 branch from 4150975 to 7756b85 Mar 3, 2016

@richsalz richsalz added the reviewed label Mar 3, 2016

@richsalz richsalz self-assigned this Mar 3, 2016

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2016

+1 will merge after anothe rreview. thanks.

@richsalz richsalz referenced this pull request Mar 3, 2016

Closed

TSA - update to SHA-2 #474

OPENSSL_free(pp);
return 0;
}

static ASN1_GENERALIZEDTIME

This comment has been minimized.

Copy link
@richsalz

richsalz Mar 3, 2016

Contributor

merge 1035/1306, put the linebreak after the paren.

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2016

any chance of tests?

@kleinmrk kleinmrk force-pushed the kleinmrk:cert_id_v2 branch from 4560de9 to 42bdc9c Mar 4, 2016

@kleinmrk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2016

I've added ess_cert_id_alg variable into existing tsa tests. I believe this is enough.

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2016

ok thanks

@jariq

This comment has been minimized.

Copy link

commented Mar 7, 2016

It's very good to see TS module is getting some attention - I've been waiting five years for inclusion of my last patch 😃. Any chance of getting this merged to 1.1?

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2016

Yes, the OpenSSL has not care very much about TS. Sorry about that.
I hope this will make it; it needs another team member to review.

@@ -635,6 +662,7 @@ void ERR_load_TS_strings(void);
# define TS_F_TS_RESP_CTX_SET_ACCURACY 128
# define TS_F_TS_RESP_CTX_SET_CERTS 129
# define TS_F_TS_RESP_CTX_SET_DEF_POLICY 130
# define TS_F_TS_RESP_CTX_SET_ESS_CERT_ID_V2_DIGEST 159

This comment has been minimized.

Copy link
@smuehlst

smuehlst Apr 5, 2016

Define TS_F_TS_RESP_CTX_SET_ESS_CERT_ID_V2_DIGEST is not used anywhere in the code.

@@ -660,12 +688,14 @@ void ERR_load_TS_strings(void);
# define TS_R_BAD_TYPE 133
# define TS_R_CANNOT_LOAD_CERT 137
# define TS_R_CANNOT_LOAD_KEY 138
# define TS_R_CANNOT_USE_SHA1_WITH_ESSCERTIDV2 140

This comment has been minimized.

Copy link
@smuehlst

smuehlst Apr 5, 2016

Define TS_R_CANNOT_USE_SHA1_WITH_ESSCERTIDV2 is not used anywhere in the code.

@mattcaswell mattcaswell added this to the Post 1.1.0 milestone May 5, 2016

@richsalz richsalz removed the feature label May 2, 2017

@levitte

This comment has been minimized.

Copy link
Member

commented May 2, 2017

For starters, it needs a rebase on top of a fresh master...

static ESS_CERT_ID_V2 *ess_CERT_ID_V2_new_init(const EVP_MD *hash_alg,
X509 *cert, int issuer_needed);
static int ess_add_signing_cert_v2(PKCS7_SIGNER_INFO *si,
ESS_SIGNING_CERT_V2 *sc);

This comment has been minimized.

Copy link
@levitte

levitte May 2, 2017

Member

Inconsistent naming... why capitalize some function names and not others?

Zoltan Glozik <zglozik@opentsa.org>. Known issues:

=over 2
=over 4

This comment has been minimized.

Copy link
@levitte

levitte May 2, 2017

Member

I think we concluded elsewhere that pointed lists should be =over 2, so please revert the change on this line.

@richsalz

This comment has been minimized.

Copy link
Contributor

commented May 2, 2017

Sigh. I feel kinda bad for the submittor. It's like we keep adding things for him to do.

@jariq

This comment has been minimized.

Copy link

commented May 2, 2017

He'll be rewarded with eternal glory.

@kleinmrk kleinmrk force-pushed the kleinmrk:cert_id_v2 branch from 009ed0d to 1da537f May 2, 2017

@kleinmrk

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2017

@levitte static functions are now consistently in lowercase.

@levitte

levitte approved these changes May 3, 2017

@levitte

This comment has been minimized.

Copy link
Member

commented May 3, 2017

Cool! Going in...

levitte added a commit that referenced this pull request May 3, 2017

Added support for ESSCertIDv2
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #771)
@levitte

This comment has been minimized.

Copy link
Member

commented May 3, 2017

Merged. I smashed it all together into one commit, f0ef20b

@levitte levitte closed this May 3, 2017

selman added a commit to Bg-Tek/openssl that referenced this pull request Jan 30, 2018

Added support for ESSCertIDv2
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from openssl#771)

mgorges added a commit to part-cw/lambdanative that referenced this pull request Feb 7, 2018

TIMESTAMP: Add error message to log if verification fails.
Zeitstempel.dfn.de currently fails verification due to a mismatch
in signatures (sha1 vs. sha2) in the new configuation of their
server. See openssl/openssl#771

@FdaSilvaYY FdaSilvaYY referenced this pull request Nov 18, 2018

Closed

This patch adds cades support for openssl #7611

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.