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

[bugfix] remove SecCertificateRef that will cause tls handshake failed #547

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yacosdad
Copy link

@yacosdad yacosdad commented Nov 9, 2019

Before deleting the 'cert' code, the package info of client for exchange had two certificates which were same with each other. The screenshot is below:
before-1
before-2
After deleting that, everything was OK!
after-1
after-2

The SecIdentityRef contains a SecCertificateRef, so we do not add another SecCertificateRef again!

Please fix it and release a new pod version, thanks!

@ckrey
Copy link
Contributor

ckrey commented Nov 9, 2019

@yacosdad Thanks for spotting and fixing this

@yacosdad
Copy link
Author

yacosdad commented Nov 11, 2019

@yacosdad Thanks for spotting and fixing this

@ckrey Em...but why the CI executed failed?Does this PR will be merged as soon as possible?

@jcavar
Copy link
Contributor

jcavar commented Nov 11, 2019

Don't worry about CI - that is just flakiness.
But should we return only cert instead of identityRef?

@yacosdad
Copy link
Author

yacosdad commented Nov 12, 2019

Don't worry about CI - that is just flakiness.
But should we return only cert instead of identityRef?

@jcavar No,we can not. The kCFStreamSSLCertificates Discussion shows that the SecIdentityRef must be first item in the array.

WechatIMG1267

@jcavar
Copy link
Contributor

jcavar commented Nov 12, 2019

Hmm, I am still not sure I understand.
Could you please share how you use certificates returned by this method?
Where do you set them?

@yacosdad
Copy link
Author

yacosdad commented Nov 12, 2019

Hmm, I am still not sure I understand.
Could you please share how you use certificates returned by this method?
Where do you set them?

  • (NSArray *)clientCertsFromP12:(NSString *)path passphrase:(NSString *)passphrase
    I use this method to general client certificate and use the result to initialize mqttsessionmanager's certificaties parameter.

Finally, the certificates was used in "open" method of MQTTSSLSecurityPolicyTransport class. That was set into the key kCFStreamSSLCertificates of ssloptions to set kCFStreamPropertySSLSettings property of CFReadStreamRef/CFWriteStreamRef.

This is the demo:
WechatIMG1269

@jcavar
Copy link
Contributor

jcavar commented Nov 12, 2019

Ok, I see. But the docs state that it should be an array of SecCertificateRefs except first SecIdentityRef. With this change, we only leave SecIdentityRef?

Also, implementation of this other library does the same thing as we are doing now:
https://github.com/daltoniam/Starscream/blob/master/Sources/Starscream/SSLClientCertificate.swift

@yacosdad
Copy link
Author

Ok, I see. But the docs state that it should be an array of SecCertificateRefs except first SecIdentityRef. With this change, we only leave SecIdentityRef?

Also, implementation of this other library does the same thing as we are doing now:
https://github.com/daltoniam/Starscream/blob/master/Sources/Starscream/SSLClientCertificate.swift

YES, my code works fine and the package screenshot in the issue topic shows that the communication process is right. You can grab the network package and take a test.

@jcavar
Copy link
Contributor

jcavar commented Nov 12, 2019

I understand what you are saying, but I don't understand why that is the case. Documentation confirms what our current code is doing. (and other library is doing the same thing).

Maybe there is an issue somewhere else?

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.

3 participants