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

CR: Adding Getters and Setters #72

Open
gfaust-qb opened this issue Aug 17, 2015 · 7 comments
Open

CR: Adding Getters and Setters #72

gfaust-qb opened this issue Aug 17, 2015 · 7 comments

Comments

@gfaust-qb
Copy link
Contributor

A similar CR to #71 but without changing members visibility (only marking them as deprecated) - the change to - at least - protected should be done in a later Version.

@Maks3w
Copy link
Contributor

Maks3w commented Aug 17, 2015

IMO the project should be rewritten with a more clear public API, typehints and contracts.

See https://github.com/Maks3w/xmldsig/blob/master/src/Adapter/AdapterInterface.phpas an example of a facade API for this project.

Probably the project could have low level classes like now and additional classes for easy consumption of the diferent standards (like XMLDsig)

@Maks3w
Copy link
Contributor

Maks3w commented Aug 17, 2015

/cc @jaimeperez Because he already propose a change in the visibility for behavior customization. May he can suggest something about how he expects public API should be.

@robrichards
Copy link
Owner

I am not considering adding more abstraction or changing interfaces for the sake of purity at the expense of performance. Will consider them on a case by case basis depending upon need/use of functionality. The library has been around for over 8 years and have yet had anyone inquire or express the need.

@gfaust-qb
Copy link
Contributor Author

It is not for the sake of purity - I plan to replace the Mcrypt-Extension and implemented a PoC for replacing the Extensions via Strategy like discussed in #38 and #55. This is a nasty thing doing it without having Getters and Setters instead of private members.

@robrichards
Copy link
Owner

Just for a POC still doesn't justify it. To replace mcrypt I am planning on just moving to leverage openssl which also will reduce dependancies. Still have never had a request nor heard a justifiable reason to support multiple crypto libs. I think the work to move from mcrypt to openssl would be much more useful imho.

@gfaust-qb
Copy link
Contributor Author

Okay, let's keep this CR in Mind and implement it when it is needed. I appreciated that there should be a Strategy to implement several Crypto-Extensions in the future (like discussed in #38).
In my opinion it is necessary to switch to Getters and Setters in the future to get an clear API and making the Library better extensible.

@jaimeperez
Copy link
Contributor

jaimeperez commented Jan 29, 2016

Hi!

Looks like I missed this thread...

In any case, I don't think it's unreasonable to support multiple crypto libs. We can see already people willing to get rid of mcrypt. We can just do that by using openssl for everything, of course, but what about those willing to use phpseclib? What about other legitimate use cases where neither mcrypt nor openssl (nor phpseclib) are enough?

Let's have an example. One of the features that we've been requested during the past months was to support in SimpleSAMLphp the use of HSMs to sign metadata or SAML messages. We don't do XML signatures or encryption in SimpleSAMLphp, that's what we use XMLSecLibs for. Similarly, XMLSecLibs does not do crypto itself, it just uses other libraries to implement crypto inside XML, which is its main purpose. Neither mcrypt nor openssl offer any capabilities to interact with an HSM over the network via PKCS#11 over a REST interface. Since that would be the only deployment scenario many could consider, the lack of support for this in the crypto libraries and the impossibility to change the crypto libraries used by XMLSecLibs, locks us out and makes it subsequently impossible for us to implement such a request.

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

No branches or pull requests

4 participants