Skip to content

Conversation

@Castaglia
Copy link
Contributor

If no serverinfo extension is found in some cases, do not abort the handshake,
but simply omit/skip that extension.

Check for already-registered serverinfo callbacks during serverinfo
registration.

Update SSL_CTX_use_serverinfo() documentation to mention the need to reload the
same serverinfo per certificate, for servers with multiple server certificates.

If no serverinfo extension is found in some cases, do not abort the handshake,
but simply omit/skip that extension.

Check for already-registered serverinfo callbacks during serverinfo
registration.

Update SSL_CTX_use_serverinfo() documentation to mention the need to reload the
same serverinfo per certificate, for servers with multiple server certificates.
@Castaglia Castaglia mentioned this pull request Mar 23, 2016
@mattcaswell
Copy link
Member

Removing +1

@mattcaswell
Copy link
Member

We need a second team review now.

@mattcaswell
Copy link
Member

Oops...added +1 to wrong PR...meant this to be for the master version...

@mattcaswell
Copy link
Member

Right, I've reviewed it for real this time... :-)

@mattcaswell
Copy link
Member

+1

@mattcaswell
Copy link
Member

Ping? Needs a second team review.

@JasonSome
Copy link

JasonSome commented Apr 22, 2016

Any second review here? Something, please?

ssl/ssl_rsa.c Outdated
custom_ext_methods *exts = &ctx->cert->srv_ext;
custom_ext_method *meth = exts->meths;

/* check for existing callbacks for this extension */
Copy link
Member

Choose a reason for hiding this comment

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

That comment is a bit confusing... what the loop does is to check if there are any extensions of the given type to see if there's any reason to add a server extension of that type.

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 be fine with that comment being removed if it comes to that)

@levitte
Copy link
Member

levitte commented Apr 29, 2016

Apart from that comment needing a rewrite, I'm fine with this.

@levitte levitte added 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 Apr 29, 2016
@levitte
Copy link
Member

levitte commented May 2, 2016

👍

@levitte levitte added the approval: done This pull request has the required number of approvals label May 2, 2016
@richsalz
Copy link
Contributor

richsalz commented May 2, 2016

merged. thanks!

@richsalz richsalz closed this May 2, 2016
@JasonSome
Copy link

@richsalz Will this become available as a bug fix on openssl 1.0.2? Because it is important for me to have it on my OpenSSL 1.0.2.

@richsalz
Copy link
Contributor

richsalz commented May 4, 2016

it is merged into 1.0.2 and will show up as part of the next bugfix release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: done This pull request has the required number of approvals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants