Allow to set custom REDIS keys for security endpoint and PSK ID

[aliakseiz]

Added setters for SEC_EP and PSKID_SEC to be able to override the default values.

[sbernard31]

Hi, thx for this PR. 🙏

Before to talk about the legal agreement issue 👇 and review the code, could you explain why you need this ? maybe you can explain a little your use case and the context where you need it ? This should help to provide a solution for your need.

[aliakseiz]

Hi,
we have a strict REDIS keys naming rules in our company and the default SEC#EP# and PSKID#SEC don't comply with it. Also I couldn't find a way to override them using current code base.

For the moment I use a custom RedisSecurityStore, which differs only by changes provided in the PR.

Wanted to avoid such a copy-pasting and to make life a bit easier when upgrading the gateway to the next milestone. Was not sure what is the correct way to do that, decided to create a PR.

By the way, ECA is settled now.

[jvermillard]

Looks useful, I'm starting to mix Leshan data structure and some other applications in the same Redis cluster, and I have the same need: a way to change the prefix to organize the data better.

That said, I'm not sure this should still be static and set by a setter, but a final attribute you can set using a constructor.

[aliakseiz]

@jvermillard thank you for suggestion, constructor is much better, indeed.
I updated PR accordingly.

[sbernard31]

we have a strict REDIS keys naming rules in our company and the default SEC#EP# and PSKID#SEC don't comply with it.

OK I understand.
This makes me think to this issue : #1249.
Do you also need this kind of feature for RedisRegistrationStore ?
@aliakseiz, just by curiosity you are using Leshan v2.0.0-Mx ?

That said, I'm not sure this should still be static and set by a setter, but a final attribute you can set using a constructor.

👍

By the way, ECA is settled now.

👍

[sbernard31]

Maybe out of scope of this PR, but I also feel that default redis key value are not so good...

Being more consistent :

1) SEC#EP# key
This is a prefix to create a key from "endpoint name" to retrieve a SecurityInfo (E.g. of key SEC#EP#my_endpoint)
(this is used with redis GET command)
So first part "SEC" is what we search and second part "EP" is the field used as index and third part "my_endpoint" is the value of the field.

2) PSKID#SEC key
Here this is the key search a value in a HashStored by field (See redis HGET command)
This allow to find "endpoint name" by "PSK Identity".
So order seems different, first part is the field index name "PSKID" and second part "SEC" the value ?
But why "SEC" as we store the "endpoint name" ?
If we want to be consistent this should be some thing like EP#PSKID ?

What do you think ? Should we at least change PSKID#SEC to EP#PSKID ?
Or any better ideas for those key name ?
Or better to not change it for backward compatibility ? (knowing that is OK to break compatibility between 1.x and 2.x and with this new PR it is easy to use old prefix)

Should we add more prefix to all key ?

Do we need a Leshan global prefix used for all store ? e.g. : LSHN#PSKID#SEC and LSHN#SEC#EP#
OR prefix by store ? e.g. : SECSTORE#PSKID#SEC and SECSTORE#SEC#EP#
OR both ? e.g. LSHN#SECSTORE#PSKID#SEC and LSHN#SECSTORE#SEC#EP#

This allow to have separate namespaces at cost of more bytes. Don't know if we should go in that way 🤷

[aliakseiz]

just by curiosity you are using Leshan v2.0.0-Mx ?

Yes, we have a gateway in production environment based on a v2.0.0-M10. Trying to keep it up to date with the latest Leshan since v1.0.0-RC2. Didn't face any critical issues so far.

This makes me think to this issue : #1249.
Do you also need this kind of feature for RedisRegistrationStore ?

Not really, but I realize that for sake of consistency and convenience it might be nice to have an ability to customize REDIS keys or at least a root prefix.
I could try extending current PR with a similar approach for RedisRegistrationStore. Or you would suggest to prepare a separate one?

[sbernard31]

Yes, we have a gateway in production environment based on a v2.0.0-M10. Trying to keep it up to date with the latest Leshan since v1.0.0-RC2. Didn't face any critical issues so far.

Good to know 🙂
Did you test master since we made lot of changes about Transport Layer Abstraction ? See #1025 (comment).

I could try extending current PR with a similar approach for RedisRegistrationStore. Or you would suggest to prepare a separate one?

I think this is better to limit this PR to SecurityStore.
Then we can do a separated PR for RegistrationStore. (It will probably lead to more question because there is a lot of argument and so maybe we need a builder or another solution ?)

Maybe we should also decide about #1398 (comment) before to move forward on RegistrationStore ? 🤔

[sbernard31]

Maybe if we need to discuss about RedisRegistrationStore we can use #1249.

[jvermillard]

@sbernard31 if you write a migration script, I'm fine with the rename to EP#PSKID 😂

[aliakseiz]

Did you test master since we made lot of changes about Transport Layer Abstraction ?

Yes, no issues so far. Migration was rather easy and straightforward.

Regarding #1398 (comment):

What do you think ? Should we at least change PSKID#SEC to EP#PSKID ?

As for me, I like the SEC#EP#key and EP#PSKID names. The order is consistent and has some logic behind. Once you get it, you can navigate easily.

Or better to not change it for backward compatibility?

Those who will experience issues with the new keys could easily override them to current ones and perform the migration later if needed.

Should we add more prefix to all key ? Do we need a Leshan global prefix used for all store?

Let's indeed continue the discussion in #1249 .

[sbernard31]

Yes, no issues so far. Migration was rather easy and straightforward.

Thx a lot for this feedback. 🙏
If you have any feedback or idea to improve about the new API, do not hesitate to share.

About EP#PSKID, your arguments make sense so let's change the key value.
Let's me know if you plan to do it in this PR ?
If yes, maybe in another commit, with javadoc explaining the break between v1.x and v2.x and old key to use for backward compatibility ?

Let's indeed continue the discussion in #1249 .

I'm waiting for your opinion about prefix. 🙂

[aliakseiz]

Let's me know if you plan to do it in this PR ?
If yes, maybe in another commit, with javadoc explaining the break between v1.x and v2.x and old key to use for backward compatibility ?

Updated the PR accordingly. Any suggestions on the wording in javadoc are welcome.

Do we need a Leshan global prefix used for all store ? e.g. : LSHN#PSKID#SEC and LSHN#SEC#EP#
OR prefix by store ? e.g. : SECSTORE#PSKID#SEC and SECSTORE#SEC#EP#
OR both ? e.g. LSHN#SECSTORE#PSKID#SEC and LSHN#SECSTORE#SEC#EP#

From my point of view, if you have a Redis instance dedicated for Leshan, prefix by store makes the most sense. Because it provides a necessary grouping and a quick overview of what is stored under those keys.

Global prefix might be useful only when Redis is shared between services, but in this case the prefix LSHN#SECSTORE#SEC#EP# most probably will not comply with your internal naming rules and you would want to use a custom prefixes anyway.

[sbernard31]

Any suggestions on the wording in javadoc are welcome

I review the last commit and it looks good to me. 👍

Just a small detail about the changes on CONTRIBUTING.md, in the future do not hesitate to create dedicated commit or even dedicated PR for this kind of "out ot topic" changes.

From my point of view, if you have a Redis instance dedicated for Leshan, prefix by store makes the most sense. Because it provides a necessary grouping and a quick overview of what is stored under those keys.

Global prefix might be useful only when Redis is shared between services, but in this case the prefix LSHN#SECSTORE#SEC#EP# most probably will not comply with your internal naming rules and you would want to use a custom prefixes anyway.

It makes sense to me. So let's add only 1 store prefix.

Remaining questions are :

1. What should we use as default prefix ? SECSTORE# or LSHNSECSTORE or something else ?
2. Do we want to add a way to set prefix ? (like adding new constructor or add parameter to existent one) OR letting user able to only change key is enough.

If we want to provide a way to set prefix we need to decide how ? none exclusive solution could be :
2.1 add a constructor RedisSecurityStore(Pool<Jedis> pool, String prefix)
2.2 add a constructor RedisSecurityStore(Pool<Jedis> pool, String prefix, String securityInfoByEndpointPrefix, String endpointByPskIdKey)
2.3 add a builder.

For prefix do you prefer to continue with this PR or a new one ?
Just by curiosity do you also plan to work on RedisRegistrationStore ?

Again thx for your contribution, that's really appreciated 🙏

[aliakseiz]

in the future do not hesitate to create dedicated commit

Sorry about that, probably a bad habit of mine from other projects. Will keep it in mind next time.

I would go for SECSTORE#, because it's shorter and still clear enough. If somebody would need to have an indication that it belongs to Leshan, it can be easily overridden via constructor.
To me it looks like using a builder here might be a bit to excessive. Not sure if the builders are widely used across Leshan and are treated as a common practice.
Maybe options 2.1 and 2.2 would be enough? First one for those who don't care about the internal naming and just need to hide the Leshan security store under some key. And second one for complete control over the keys and prefixes.

I will update this PR with prefix setting.

Just by curiosity do you also plan to work on RedisRegistrationStore ?

Yes, right after finishing current PR.

[sbernard31]

Sorry about that,

Not a big deal 😉

I would go for SECSTORE#

👍

Not sure if the builders are widely used across Leshan and are treated as a common practice.

I can confirm Leshan code base has several Builder. There are generally added when there is too many parameters in constructors or too many constructors.
I know Builder has many drawback but I didn't find better way to create instance of a class with many final parameters.

To me it looks like using a builder here might be a bit to excessive.

I agree adding builder for RedisSecurityStore seems a bit overkill.
But my guess is that for RedisRegisrationStore this will be hard to avoid Builder because there is so many parameters.
So one reason to use Builder in RedisSecutiryStore could be for API consistency with RedisRegisrationStore (of course this point makes sense only if we decide to use builder in RedisRegisrationStore)

I just said that to let you know the big picture.
On my side, I have no strong opinion between builder vs more constructor in RedisSecurityStore.
So, feel free to add constructor or builder.

Maybe options 2.1 and 2.2 would be enough? First one for those who don't care about the internal naming and just need to hide the Leshan security store under some key. And second one for complete control over the keys and prefixes.

If you chose constructor, this makes sense to me.

Yes, right after finishing current PR.

Great 👍

[aliakseiz]

...for API consistency with RedisRegisrationStore

That was exactly what I thought about right after sending my previous comment 🙂
Had some doubts regarding usage mostly because I am not familiar with the Leshan codebase yet.

because there is so many parameters

I totally agree with you and will add a builder then.

[github-actions]

ℹ️ Some tips :

• Once you fix your issues locally, do not create a new PR, see how to refine your PR.
• See How to contribute guide and some Code & design guidelines.

If documentation or those automatic comments are not clear enough, please create a new issue to discus about how to enhance it.

[github-actions]

❌ Java Import are not sorted ! (more details)

Ensure your code build locally using:

mvn clean install

Or just validate java import with :

mvn impsort:check

You can sort Java import with :

mvn impsort:sort

See also How configure your IDE.

[aliakseiz]

Thank you @sbernard31 , those are all valid points and suggestions.
I think I fixed all of them, please take a look again.

[sbernard31]

Except : #1398 (comment), this looks good to me 👍

About sortimport issue, It looks strange so, ignore it for now. I will look at this.
I ran mvn impsort:check and it sounds good to me 🤔

[sbernard31]

I didn't notice that your Builder is not fluent.
Are you OK if we change it in that way ? (each setter must return the builder so you can write)

securityStore = new RedisSecurityStore.Builder(jedis).
       .setPrefix(null) //
       .setEndpointByPskIdKey("EPKEY") //
       .setSecurityInfoByEndpointPrefix("SECKey").build();

[aliakseiz]

I didn't notice that you Builder is not fluent.

Don't know what I was thinking of, of course setters should return Builder instance. Fixed it.

[sbernard31]

✔️ I think we're good now.

I play a bit with it and it seems to work, I also tested it locally with mvn clean install javadoc:javadoc -Predis and all seems good 👍 !

I will now integrated this in master.

Thank you very much for this contribution 🙏

[sbernard31]

(as I said we will ignore the github action issue, I haven't time to go deeper in it and didn't find any obvious way to fix it)

[aliakseiz]

I think we're good now.

Thank you for all your advice and for patience 👍

I will start #1249 now.

[sbernard31]

Thank you for all your advice and for patience +1

You're welcome.

I will start #1249 now.

👍

[sbernard31]

This is now integrated in master (in squashed commit fd5c9c0 + typo commit b7dc3cf)

I notice that your email is @any-ware.nl. Fun fact, when I started to work on Eclipse project my company was named anyware-technology. It seems to be a common name in IoT 😁