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

Expand SSLSessionId structure to manage allocated memory. #1614

Merged
merged 5 commits into from
Feb 12, 2019

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Feb 10, 2019

No description provided.

@slaff slaff added this to the 3.7.2 milestone Feb 11, 2019
typedef struct {
/** @brief Manages buffer to store SSL Session ID
*/
struct SSLSessionId {
Copy link
Contributor

Choose a reason for hiding this comment

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

The TcpConnection.h is starting to have too much code that is not strictly Tcp related but SSL related. Can you put SSLSessionId, SslFingerprintType, SSLFingerprints and SSLKeyCertPair into a separate header file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put SSLSessionId, SslFingerprintType, SSLFingerprints and SSLKeyCertPair into a separate header file?

Sorry, I was not clear enough. I've meant one header file and not header file per structure. Can you put them back to one header file OR create a separate header file in which all new SSL related header files are included?

Copy link
Contributor Author

@mikee47 mikee47 Feb 11, 2019

Choose a reason for hiding this comment

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

I'll be elaborating on SSLFingerprints so thought it best to split them functionally - it's like the 'fingerprint API' header. Yes, can certainly use a common header - is it worth having an SSL sub-folder perhaps? Could add SslValidator into that.

Copy link
Contributor

@slaff slaff left a comment

Choose a reason for hiding this comment

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

The code looks good. Just move the SSL related code to a separate header file.

@mikee47
Copy link
Contributor Author

mikee47 commented Feb 11, 2019

It occurred to me doing all this that all three structures could be more easily implemented using Strings...

@slaff
Copy link
Contributor

slaff commented Feb 11, 2019

is it worth having an SSL sub-folder perhaps?

Sounds good to me. In it you can put also the SSL Validator stuff.

It occurred to me doing all this that all three structures could be more easily implemented using String

That would be great. As long as it simplifies the code.

@mikee47
Copy link
Contributor Author

mikee47 commented Feb 11, 2019

There are a few naming inconsistencies:

SslValidatorCallback -> SSLValidatorCallback
SslFingerprintType -> SSLFingerprintType
SslValidator.h -> SSLValidator.h
SslValidator.cpp -> SSLValidator.cpp

Do you want those changed, or left as-is?

@slaff
Copy link
Contributor

slaff commented Feb 11, 2019

Do you want those changed, or left as-is?

Yes, please. Make them consistent. Since we don't use all caps for abbreviations, for example for TCP* we use Tcp*, I would suggest that we follow the same rule also for SSL. That said SSL* should be Ssl*.
At the moment there are expectations from the rule, like DNSServer instead of DnsServer but that should not be part of this PR.

@mikee47 mikee47 force-pushed the fix/SSLSessionId branch 2 times, most recently from a7aa4ac to e26406b Compare February 11, 2019 17:21
SSLFingerprints -> SslFingerprints
SSLKeyCertPair -> SslKeyCertPair
SSL SessionId -> SslSessionId

Definitions included for old versions but will generate compiler warning.
Change from `struct` to `class`. Data no longer directly accessible.
@slaff
Copy link
Contributor

slaff commented Feb 12, 2019

@mikee47 Mike, before I merge this PR can you provide a list of incompatible changes?
For example keyPassword is no longer public and in order to access it one needs to use getKeyPassword().

@mikee47
Copy link
Contributor Author

mikee47 commented Feb 12, 2019

@slaff None of these changes should break users' applications as neither SSLSessionId nor SSLKeyCertPair are used in any user APIs. SSLFingerprintType is used as an API parameter so it's possible some user apps have it as well. Shall I add a deprecated version of that to the PR?

Possible breaking changes:

struct SSLKeyCertPair -> class SslKeyCertPair
	certificate -> getCertificate()
	certificateLength -> getCertificateLength()
	key -> getKey()
	keyLength -> getKeyLength()
struct SSLSessionId -> class SslSessionId
	value -> getValue()
	length -> getLength()
SSLFingerprintType -> SslFingerprintType

Deprecated so old version still works with compiler warning:

SSLFingerprints -> SslFingerprints
SSLKeyCertPair -> SslKeyCertPair
SSLSessionId -> SslSessionId

@slaff slaff merged commit 88a080b into SmingHub:develop Feb 12, 2019
@mikee47 mikee47 deleted the fix/SSLSessionId branch February 12, 2019 14:15
@slaff slaff mentioned this pull request Feb 27, 2019
4 tasks
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.

2 participants