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

Allow users to inherit and override CertStore #7827

Merged
merged 13 commits into from
Jan 18, 2021
Merged

Allow users to inherit and override CertStore #7827

merged 13 commits into from
Jan 18, 2021

Conversation

paulocsanz
Copy link
Contributor

Fixes: #7826

This seems the minimal change that is enough to avoid guard headers hacks and allow users to have a more customized CertStore.

@paulocsanz
Copy link
Contributor Author

I've been thinking if it's not better to make a CertStoreBase with a virtual installCertStore, make CertStore inherit from and basically not modify it. And where CertStore is passed we can pass a CertStoreBase.

What are the thoughts of the community?

I can modify my PR accordingly.

@earlephilhower
Copy link
Collaborator

I've been thinking if it's not better to make a CertStoreBase with a virtual installCertStore, make CertStore inherit from and basically not modify it. And where CertStore is passed we can pass a CertStoreBase.

That seems like a reasonable thing to do and would avoid dragging unneeded class variables into subclasses. But, I'm sure you are trying to get some other way to hold the certstore (i.e. a PROGMEM array), so if you can get that in it would really be nice.

@paulocsanz
Copy link
Contributor Author

paulocsanz commented Jan 18, 2021

Perfect, I've implemented it.

I've added a virtual destructor to CertStoreBase even tough we only borrow it, never own as CertStoreBase. And that CertStore should be long-lived and probably won't ever be destroyed.

But Idk about c++ best practices surrounding virtual destructors. It seemed like the best choice. But I can remove it.

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

Looks good. Did you want to add in your other CertStore object?

@paulocsanz
Copy link
Contributor Author

paulocsanz commented Jan 18, 2021

I don't think it's the best move stabilizing another CertStore alternative, with its own python script. I can provide the alternative if it's deemed important since so many people depend on the hacks and save certs to PROGMEM.

But I'm not really confortable doing it right now, but I can indeed provide it in a future PR.

@earlephilhower earlephilhower merged commit 85e2fff into esp8266:master Jan 18, 2021
@maakbaas
Copy link

@paulocsanz: Thanks for adding this! It seems that this change was at least partly inspired by my hack, which indeed came about due to the lack of virtual methods. I have updated my code with the new inheritance mechanism in this commit: maakbaas/esp8266-iot-framework@8a752f8

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.

Unable to override CertStore
3 participants