search

robrichards / xmlseclibs

A PHP library for XML Security
BSD 3-Clause "New" or "Revised" License
384 stars 180 forks source link

CR: Adding Getters and Setters #72

Open gfaust-qb opened 8 years ago

gfaust-qb commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

/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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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.