Alex, Denis,
Seems like security API is indeed a bit over-engineered. Let's get rid of SecurityContext and use SecuritySubject instead. > SecurityContext is just a POJO wrapper over > SecuritySubject's > org.apache.ignite.plugin.security.SecuritySubject#permissions. > It's functionality can be easily moved to SecuritySubject. I totally agree. Both subject and context are implemented by plugin provider, and I don't see any reason to keep both abstractions, especially if we are going to get rid of transferring subject in node attributes (argument that subject is more lightweight won't work anymore). Also, there's kind of mess in node authentication logic. There are at least two components responsible for it: DiscoverySpiNodeAuthenticator (which is forcibly set by GridDiscoveryManager, but in fact public) and GridSecurityProcessor (which performs actual node auth logic, but private). I also don't understand why we need both #authenticate(AuthenticationContext) and #authenticateNode(ClusterNode, SecurityCredentials) methods while it's possible to set explicit SecuritySubjectType.REMOTE_NODE in AuthenticationContext (this is arguable; perhaps there are strong reasons). Finally, areas of responsibility between IgniteSecurity and GridSecurityProcessor are kind of mixed. As far as I understand, the first is responsible for Ignite-internal management of security logic (keeping thread-local context, caching security contexts, etc; we don't expect IgniteSecurity to be replaced by plugin provider) and the latter is responsible for user-custom authentication / authorization logic. To be honest, it took plenty of time to figure this out for me. From my point of view, we should make GridSecurityProcessor interface public, rename it (it requires plenty of time to find the difference from IgniteSecurity), make its API as simple and non-duplicating as possible and clarify its area of responsibility (e.g. should it be responsible for propagation of successfully authenticated subject among all nodes or not?) to make it easy to embed custom security logic in Ignite. Regarding thin clients fix: implementation made by Denis suits better to the very implicit contract that it's better to change API contracts of an internal IgniteSecurity than of internal GridSecurityProcessor (which actually mustn't be internal). > My approach doesn't require any IEPs, just minor change in code and to > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > contract. Looks like a misuse of #authenticate method to me. It should perform initial authentication based on credentials (this may include queries to external authentication subsystem, e.g. LDAP). User may want to don't authenticate thin client on every node (this will increase the number of requests to auth subsystem unless user implicitly implements propagation of thin clients' contexts between nodes and make #authenticate cluster-wide idempotent: first call should perform actual authentication, next calls should retrieve context of already authenticated client). Presence of the separate #securityContext(UUID) highlights that user indeed should care about propagation of thin clients' contexts between the cluster nodes. -- Ivan On Fri, Mar 20, 2020 at 12:22 PM Veena Mithare <[hidden email]> wrote: > Hi Alexei, Denis, > > One of the main usecases of thin client authentication is to be able to > audit the changes done using the thin client user. > To enable that : > We really need to resolve this concern as well : > https://issues.apache.org/jira/browse/IGNITE-12781 > > ( Incorrect security subject id is associated with a cache_put event > when the originator of the event is a thin client. ) > > Regards, > Veena > > > -----Original Message----- > From: Alexei Scherbakov <[hidden email]> > Sent: 18 March 2020 08:11 > To: dev <[hidden email]> > Subject: Re: Security Subject of thin client on remote nodes > > Denis Garus, > > Both variants are capable of solving the thin client security context > problem. > > My approach doesn't require any IEPs, just minor change in code and to > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > contract. > We can add appropriate documentation to emphasize this. > The argument "fragile" is not very convincing for me. > > I think we should collect more opinions before proceeding with IEP. > > Considering a fact we actually *may not care* about compatibility (I've > already explained why), I'm thinking of another approach. > Let's get rid of SecurityContext and use SecuritySubject instead. > SecurityContext is just a POJO wrapper over SecuritySubject's > org.apache.ignite.plugin.security.SecuritySubject#permissions. > It's functionality can be easily moved to SecuritySubject. > > What do you think? > > > > пн, 16 мар. 2020 г. в 15:47, Denis Garus <[hidden email]>: > > > Hello, Alexei! > > > > I agree with you if we may not care about compatibility at all, then > > we can solve the problem much more straightforward way. > > > > In your case, the method GridSecurityProcessor#authenticate will have > > an implicit contract: > > [ if actx.subject() != null then > > returns SecurityContext > > else > > do authenticate ] > > > > It looks fragile. > > > > When we extend the GridSecurityProcessor, there isn't this problem: > > we have the explicit contract and can make default implementation that > > throws an unsupported operation exception to enforcing compatibility > > check. > > > > In any case, we need to change GridSecurityProcessor implementation. > > > > But I think your proposal to try to find a security context in the > > node's attributes first is right for backward compatibility when > > Ignite users don't use thin clients. > > > > Summary: > > I suggest adding a new method to GridSecurityProcessor because it has > > a clear contract and enforces compatibility check natural way. > > > > вс, 15 мар. 2020 г. в 17:13, Alexei Scherbakov < > > [hidden email] > > >: > > > > > Denis Garus, > > > > > > I've looked at the IEP proposed by you and currently I'm thinking > > > it's > > not > > > immediately required. > > > > > > The problem of missing SecurityContexts of thin clients can be > > > solved > > much > > > easily. > > > > > > Below is the stub of a fix, it requires correct implementation of > > > method > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > #authenticatedSubject > > > by GridSecurityProcessor: > > > > > > /** {@inheritDoc} */ > > > @Override public OperationSecurityContext withContext(UUID nodeId) > { > > > try { > > > SecurityContext ctx0 = secCtxs.get(nodeId); > > > > > > if (ctx0 == null) { > > > ClusterNode node = > > > Optional.ofNullable(ctx.discovery().node(nodeId)) > > > .orElseGet(() -> > > > ctx.discovery().historicalNode(nodeId)); > > > > > > // This is a cluster node. > > > if (node != null) > > > ctx0 = nodeSecurityContext(marsh, > > > U.resolveClassLoader(ctx.config()), findNode(nodeId)); > > > else { > > > // This is already authenticated thin client. > > > SecuritySubject subj = > > > authenticatedSubject(nodeId); > > > > > > assert subj != null : "Subject is null " + > > > nodeId; > > > > > > AuthenticationContext actx = new > > > AuthenticationContext(); > > > actx.subject(subj); > > > > > > ctx0 = secPrc.authenticate(actx); > > > } > > > } > > > > > > secCtxs.putIfAbsent(nodeId, ctx0); > > > > > > return withContext(ctx0); > > > } catch (IgniteCheckedException e) { > > > throw new IgniteException(e); > > > } > > > > > > The idea is to create a thin client SecurityContext on a node not > > > having > > a > > > local context using existing SecuritySubject data. > > > > > > Method > > > > > org.apache.ignite.internal.processors.security.GridSecurityProcessor#a > > uthenticate > > > should check for not null SecuritySubject field and just recreate > > > SecurityContext using passed info (because it's already authenticated). > > > > > > We have all necessary information in SecuritySubject returned by > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > #authenticatedSubject > > > by GridSecurityProcessor method. > > > > > > Because it is internal API, we may not care about compatibility at > > > all, but nevertheless it is possible to add compatibility check in > > > the method above. If a feature is not supported the operations from > > > thin clients should be forbidden. > > > > > > You proposal has the similar problem: if GridSecurityProcessor does > > > not support retriving context for thin clients, such clients will > > > not be able to proceed with operation. > > > > > > Still, the cleanup of security API is necessary and should be done > > > in 3.0 > > > > > > > > > > > > чт, 12 мар. 2020 г. в 16:48, VeenaMithare <[hidden email]>: > > > > > > > HI , > > > > > > > > Created this jira : > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > > > > > regards, > > > > Veena. > > > > > > > > > > > > > > > > -- > > > > Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ > > > > > > > > > > > > > -- > > > > > > Best regards, > > > Alexei Scherbakov > > > > > > > > -- > > Best regards, > Alexei Scherbakov > ________________________________ > > > Spread bets and CFDs are complex instruments and come with a high risk of > losing money rapidly due to leverage. 70.5% of retail investor accounts > lose money when spread betting and/or trading CFDs with this provider. You > should consider whether you understand how spread bets and CFDs work and > whether you can afford to take the high risk of losing your money. > > Professional clients: Losses can exceed deposits when spread betting and > trading CFDs. Countdowns carry a level of risk to your capital as you could > lose all of your investment. These products may not be suitable for all > clients therefore ensure you understand the risks and seek independent > advice. Invest only what you can afford to lose. > > CMC Markets UK plc (173730) and CMC Spreadbet plc (170627) are authorised > and regulated by the Financial Conduct Authority in the United Kingdom. CMC > Markets UK plc and CMC Spreadbet plc are registered in England and Wales > with Company Numbers 02448409 and 02589529 and with their registered > offices at 133 Houndsditch, London, EC3A 7BX. > > The content of this e-mail (including any attachments) is strictly > confidential and is for the sole use of the intended addressee(s). If you > are not the intended recipient of this e-mail please notify the sender > immediately and delete this e-mail from your system. Any disclosure, > copying, dissemination or use of its content (including any attachments) is > strictly prohibited. CMC Markets UK plc and CMC Spreadbet plc reserve the > right to intercept and monitor the content of the e-mail messages to and > from its systems. > > E-mails may be interfered with or may contain viruses or other defects for > which CMC Markets UK plc and CMC Spreadbet plc accept no responsibility. It > is the responsibility of the recipient to carry out a virus check on the > e-mail and any attachment(s). > > This communication is not intended as an offer or solicitation for the > purchase or sale of a financial instrument or as an official confirmation > of any transaction unless specifically presented as such. > > CMC Markets UK plc and CMC Spreadbet plc are registered in England and > Wales with Company Numbers 02448409 and 02589529 and with their registered > offices at 133 Houndsditch, London, EC3A 7BX. > |
Hello, Alexei, Ivan!
>> Seems like security API is indeed a bit over-engineered Nobody has doubt we should do a reworking of GridSecurityProcessor. But this point is outside of scope thin client's problem that we are solving. I think we can create new IEP that will accumulate all ideas of Ignite's security improvements. >> Presence of the separate #securityContext(UUID) highlights that user indeed should care >> about propagation of thin clients' contexts between the cluster nodes. I agree with Ivan. I've implemented both variants, and I like one with #securityContext(UUID) more. Could you please take a look at PR [1] for the issue [2]? 1. https://github.com/apache/ignite/pull/7523 2. https://issues.apache.org/jira/browse/IGNITE-12759 пн, 23 мар. 2020 г. в 11:45, Ivan Rakov <[hidden email]>: > Alex, Denis, > > Seems like security API is indeed a bit over-engineered. > > Let's get rid of SecurityContext and use SecuritySubject instead. > > SecurityContext is just a POJO wrapper over > > SecuritySubject's > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > > It's functionality can be easily moved to SecuritySubject. > > I totally agree. Both subject and context are implemented by plugin > provider, and I don't see any reason to keep both abstractions, especially > if we are going to get rid of transferring subject in node attributes > (argument that subject is more lightweight won't work anymore). > > Also, there's kind of mess in node authentication logic. There are at least > two components responsible for it: DiscoverySpiNodeAuthenticator (which is > forcibly set by GridDiscoveryManager, but in fact public) and > GridSecurityProcessor (which performs actual node auth logic, but private). > I also don't understand why we need both > #authenticate(AuthenticationContext) and #authenticateNode(ClusterNode, > SecurityCredentials) methods while it's possible to set explicit > SecuritySubjectType.REMOTE_NODE in AuthenticationContext (this is arguable; > perhaps there are strong reasons). > > Finally, areas of responsibility between IgniteSecurity and > GridSecurityProcessor are kind of mixed. As far as I understand, the first > is responsible for Ignite-internal management of security logic (keeping > thread-local context, caching security contexts, etc; we don't expect > IgniteSecurity to be replaced by plugin provider) and the latter is > responsible for user-custom authentication / authorization logic. To be > honest, it took plenty of time to figure this out for me. > > From my point of view, we should make GridSecurityProcessor interface > public, rename it (it requires plenty of time to find the difference from > IgniteSecurity), make its API as simple and non-duplicating as possible and > clarify its area of responsibility (e.g. should it be responsible for > propagation of successfully authenticated subject among all nodes or not?) > to make it easy to embed custom security logic in Ignite. > > Regarding thin clients fix: implementation made by Denis suits better to > the very implicit contract that it's better to change API contracts of an > internal IgniteSecurity than of internal GridSecurityProcessor (which > actually mustn't be internal). > > > My approach doesn't require any IEPs, just minor change in code and to > > > > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > > contract. > > Looks like a misuse of #authenticate method to me. It should perform > initial authentication based on credentials (this may include queries to > external authentication subsystem, e.g. LDAP). User may want to don't > authenticate thin client on every node (this will increase the number of > requests to auth subsystem unless user implicitly implements propagation of > thin clients' contexts between nodes and make #authenticate cluster-wide > idempotent: first call should perform actual authentication, next calls > should retrieve context of already authenticated client). Presence of the > separate #securityContext(UUID) highlights that user indeed should care > about propagation of thin clients' contexts between the cluster nodes. > > -- > Ivan > > On Fri, Mar 20, 2020 at 12:22 PM Veena Mithare <[hidden email]> > wrote: > > > Hi Alexei, Denis, > > > > One of the main usecases of thin client authentication is to be able to > > audit the changes done using the thin client user. > > To enable that : > > We really need to resolve this concern as well : > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > ( Incorrect security subject id is associated with a cache_put event > > when the originator of the event is a thin client. ) > > > > Regards, > > Veena > > > > > > -----Original Message----- > > From: Alexei Scherbakov <[hidden email]> > > Sent: 18 March 2020 08:11 > > To: dev <[hidden email]> > > Subject: Re: Security Subject of thin client on remote nodes > > > > Denis Garus, > > > > Both variants are capable of solving the thin client security context > > problem. > > > > My approach doesn't require any IEPs, just minor change in code and to > > > > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > > contract. > > We can add appropriate documentation to emphasize this. > > The argument "fragile" is not very convincing for me. > > > > I think we should collect more opinions before proceeding with IEP. > > > > Considering a fact we actually *may not care* about compatibility (I've > > already explained why), I'm thinking of another approach. > > Let's get rid of SecurityContext and use SecuritySubject instead. > > SecurityContext is just a POJO wrapper over SecuritySubject's > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > > It's functionality can be easily moved to SecuritySubject. > > > > What do you think? > > > > > > > > пн, 16 мар. 2020 г. в 15:47, Denis Garus <[hidden email]>: > > > > > Hello, Alexei! > > > > > > I agree with you if we may not care about compatibility at all, then > > > we can solve the problem much more straightforward way. > > > > > > In your case, the method GridSecurityProcessor#authenticate will have > > > an implicit contract: > > > [ if actx.subject() != null then > > > returns SecurityContext > > > else > > > do authenticate ] > > > > > > It looks fragile. > > > > > > When we extend the GridSecurityProcessor, there isn't this problem: > > > we have the explicit contract and can make default implementation that > > > throws an unsupported operation exception to enforcing compatibility > > > check. > > > > > > In any case, we need to change GridSecurityProcessor implementation. > > > > > > But I think your proposal to try to find a security context in the > > > node's attributes first is right for backward compatibility when > > > Ignite users don't use thin clients. > > > > > > Summary: > > > I suggest adding a new method to GridSecurityProcessor because it has > > > a clear contract and enforces compatibility check natural way. > > > > > > вс, 15 мар. 2020 г. в 17:13, Alexei Scherbakov < > > > [hidden email] > > > >: > > > > > > > Denis Garus, > > > > > > > > I've looked at the IEP proposed by you and currently I'm thinking > > > > it's > > > not > > > > immediately required. > > > > > > > > The problem of missing SecurityContexts of thin clients can be > > > > solved > > > much > > > > easily. > > > > > > > > Below is the stub of a fix, it requires correct implementation of > > > > method > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > > #authenticatedSubject > > > > by GridSecurityProcessor: > > > > > > > > /** {@inheritDoc} */ > > > > @Override public OperationSecurityContext withContext(UUID > nodeId) > > { > > > > try { > > > > SecurityContext ctx0 = secCtxs.get(nodeId); > > > > > > > > if (ctx0 == null) { > > > > ClusterNode node = > > > > Optional.ofNullable(ctx.discovery().node(nodeId)) > > > > .orElseGet(() -> > > > > ctx.discovery().historicalNode(nodeId)); > > > > > > > > // This is a cluster node. > > > > if (node != null) > > > > ctx0 = nodeSecurityContext(marsh, > > > > U.resolveClassLoader(ctx.config()), findNode(nodeId)); > > > > else { > > > > // This is already authenticated thin client. > > > > SecuritySubject subj = > > > > authenticatedSubject(nodeId); > > > > > > > > assert subj != null : "Subject is null " + > > > > nodeId; > > > > > > > > AuthenticationContext actx = new > > > > AuthenticationContext(); > > > > actx.subject(subj); > > > > > > > > ctx0 = secPrc.authenticate(actx); > > > > } > > > > } > > > > > > > > secCtxs.putIfAbsent(nodeId, ctx0); > > > > > > > > return withContext(ctx0); > > > > } catch (IgniteCheckedException e) { > > > > throw new IgniteException(e); > > > > } > > > > > > > > The idea is to create a thin client SecurityContext on a node not > > > > having > > > a > > > > local context using existing SecuritySubject data. > > > > > > > > Method > > > > > > > org.apache.ignite.internal.processors.security.GridSecurityProcessor#a > > > uthenticate > > > > should check for not null SecuritySubject field and just recreate > > > > SecurityContext using passed info (because it's already > authenticated). > > > > > > > > We have all necessary information in SecuritySubject returned by > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > > #authenticatedSubject > > > > by GridSecurityProcessor method. > > > > > > > > Because it is internal API, we may not care about compatibility at > > > > all, but nevertheless it is possible to add compatibility check in > > > > the method above. If a feature is not supported the operations from > > > > thin clients should be forbidden. > > > > > > > > You proposal has the similar problem: if GridSecurityProcessor does > > > > not support retriving context for thin clients, such clients will > > > > not be able to proceed with operation. > > > > > > > > Still, the cleanup of security API is necessary and should be done > > > > in 3.0 > > > > > > > > > > > > > > > > чт, 12 мар. 2020 г. в 16:48, VeenaMithare <[hidden email] > >: > > > > > > > > > HI , > > > > > > > > > > Created this jira : > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > > > > > > > regards, > > > > > Veena. > > > > > > > > > > > > > > > > > > > > -- > > > > > Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ > > > > > > > > > > > > > > > > > -- > > > > > > > > Best regards, > > > > Alexei Scherbakov > > > > > > > > > > > > > -- > > > > Best regards, > > Alexei Scherbakov > > ________________________________ > > > > > > Spread bets and CFDs are complex instruments and come with a high risk of > > losing money rapidly due to leverage. 70.5% of retail investor accounts > > lose money when spread betting and/or trading CFDs with this provider. > You > > should consider whether you understand how spread bets and CFDs work and > > whether you can afford to take the high risk of losing your money. > > > > Professional clients: Losses can exceed deposits when spread betting and > > trading CFDs. Countdowns carry a level of risk to your capital as you > could > > lose all of your investment. These products may not be suitable for all > > clients therefore ensure you understand the risks and seek independent > > advice. Invest only what you can afford to lose. > > > > CMC Markets UK plc (173730) and CMC Spreadbet plc (170627) are authorised > > and regulated by the Financial Conduct Authority in the United Kingdom. > CMC > > Markets UK plc and CMC Spreadbet plc are registered in England and Wales > > with Company Numbers 02448409 and 02589529 and with their registered > > offices at 133 Houndsditch, London, EC3A 7BX. > > > > The content of this e-mail (including any attachments) is strictly > > confidential and is for the sole use of the intended addressee(s). If you > > are not the intended recipient of this e-mail please notify the sender > > immediately and delete this e-mail from your system. Any disclosure, > > copying, dissemination or use of its content (including any attachments) > is > > strictly prohibited. CMC Markets UK plc and CMC Spreadbet plc reserve the > > right to intercept and monitor the content of the e-mail messages to and > > from its systems. > > > > E-mails may be interfered with or may contain viruses or other defects > for > > which CMC Markets UK plc and CMC Spreadbet plc accept no responsibility. > It > > is the responsibility of the recipient to carry out a virus check on the > > e-mail and any attachment(s). > > > > This communication is not intended as an offer or solicitation for the > > purchase or sale of a financial instrument or as an official confirmation > > of any transaction unless specifically presented as such. > > > > CMC Markets UK plc and CMC Spreadbet plc are registered in England and > > Wales with Company Numbers 02448409 and 02589529 and with their > registered > > offices at 133 Houndsditch, London, EC3A 7BX. > > > |
Why can't we start gradually changing security API right now ?
I see no point in delaying with. All changes will go to next 2.9 release anyway. My proposal: 1. Get rid of security context. Doing this will bring security API to more or less consistent state. 2. Remove IEP-41 because it's no longer needed because of change [1] 3. Propose an IEP to make security API avoid using internals. пн, 23 мар. 2020 г. в 19:53, Denis Garus <[hidden email]>: > Hello, Alexei, Ivan! > > >> Seems like security API is indeed a bit over-engineered > > Nobody has doubt we should do a reworking of GridSecurityProcessor. > But this point is outside of scope thin client's problem that we are > solving. > I think we can create new IEP that will accumulate all ideas of Ignite's > security improvements. > > >> Presence of the separate #securityContext(UUID) highlights that user > indeed should care > >> about propagation of thin clients' contexts between the cluster nodes. > > I agree with Ivan. I've implemented both variants, > and I like one with #securityContext(UUID) more. > > Could you please take a look at PR [1] for the issue [2]? > > 1. https://github.com/apache/ignite/pull/7523 > 2. https://issues.apache.org/jira/browse/IGNITE-12759 > > пн, 23 мар. 2020 г. в 11:45, Ivan Rakov <[hidden email]>: > > > Alex, Denis, > > > > Seems like security API is indeed a bit over-engineered. > > > > Let's get rid of SecurityContext and use SecuritySubject instead. > > > SecurityContext is just a POJO wrapper over > > > SecuritySubject's > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > > > It's functionality can be easily moved to SecuritySubject. > > > > I totally agree. Both subject and context are implemented by plugin > > provider, and I don't see any reason to keep both abstractions, > especially > > if we are going to get rid of transferring subject in node attributes > > (argument that subject is more lightweight won't work anymore). > > > > Also, there's kind of mess in node authentication logic. There are at > least > > two components responsible for it: DiscoverySpiNodeAuthenticator (which > is > > forcibly set by GridDiscoveryManager, but in fact public) and > > GridSecurityProcessor (which performs actual node auth logic, but > private). > > I also don't understand why we need both > > #authenticate(AuthenticationContext) and #authenticateNode(ClusterNode, > > SecurityCredentials) methods while it's possible to set explicit > > SecuritySubjectType.REMOTE_NODE in AuthenticationContext (this is > arguable; > > perhaps there are strong reasons). > > > > Finally, areas of responsibility between IgniteSecurity and > > GridSecurityProcessor are kind of mixed. As far as I understand, the > first > > is responsible for Ignite-internal management of security logic (keeping > > thread-local context, caching security contexts, etc; we don't expect > > IgniteSecurity to be replaced by plugin provider) and the latter is > > responsible for user-custom authentication / authorization logic. To be > > honest, it took plenty of time to figure this out for me. > > > > From my point of view, we should make GridSecurityProcessor interface > > public, rename it (it requires plenty of time to find the difference from > > IgniteSecurity), make its API as simple and non-duplicating as possible > and > > clarify its area of responsibility (e.g. should it be responsible for > > propagation of successfully authenticated subject among all nodes or > not?) > > to make it easy to embed custom security logic in Ignite. > > > > Regarding thin clients fix: implementation made by Denis suits better to > > the very implicit contract that it's better to change API contracts of an > > internal IgniteSecurity than of internal GridSecurityProcessor (which > > actually mustn't be internal). > > > > > My approach doesn't require any IEPs, just minor change in code and to > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > > > contract. > > > > Looks like a misuse of #authenticate method to me. It should perform > > initial authentication based on credentials (this may include queries to > > external authentication subsystem, e.g. LDAP). User may want to don't > > authenticate thin client on every node (this will increase the number of > > requests to auth subsystem unless user implicitly implements propagation > of > > thin clients' contexts between nodes and make #authenticate cluster-wide > > idempotent: first call should perform actual authentication, next calls > > should retrieve context of already authenticated client). Presence of the > > separate #securityContext(UUID) highlights that user indeed should care > > about propagation of thin clients' contexts between the cluster nodes. > > > > -- > > Ivan > > > > On Fri, Mar 20, 2020 at 12:22 PM Veena Mithare <[hidden email] > > > > wrote: > > > > > Hi Alexei, Denis, > > > > > > One of the main usecases of thin client authentication is to be able to > > > audit the changes done using the thin client user. > > > To enable that : > > > We really need to resolve this concern as well : > > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > > > ( Incorrect security subject id is associated with a cache_put event > > > when the originator of the event is a thin client. ) > > > > > > Regards, > > > Veena > > > > > > > > > -----Original Message----- > > > From: Alexei Scherbakov <[hidden email]> > > > Sent: 18 March 2020 08:11 > > > To: dev <[hidden email]> > > > Subject: Re: Security Subject of thin client on remote nodes > > > > > > Denis Garus, > > > > > > Both variants are capable of solving the thin client security context > > > problem. > > > > > > My approach doesn't require any IEPs, just minor change in code and to > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > > > contract. > > > We can add appropriate documentation to emphasize this. > > > The argument "fragile" is not very convincing for me. > > > > > > I think we should collect more opinions before proceeding with IEP. > > > > > > Considering a fact we actually *may not care* about compatibility (I've > > > already explained why), I'm thinking of another approach. > > > Let's get rid of SecurityContext and use SecuritySubject instead. > > > SecurityContext is just a POJO wrapper over SecuritySubject's > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > > > It's functionality can be easily moved to SecuritySubject. > > > > > > What do you think? > > > > > > > > > > > > пн, 16 мар. 2020 г. в 15:47, Denis Garus <[hidden email]>: > > > > > > > Hello, Alexei! > > > > > > > > I agree with you if we may not care about compatibility at all, then > > > > we can solve the problem much more straightforward way. > > > > > > > > In your case, the method GridSecurityProcessor#authenticate will have > > > > an implicit contract: > > > > [ if actx.subject() != null then > > > > returns SecurityContext > > > > else > > > > do authenticate ] > > > > > > > > It looks fragile. > > > > > > > > When we extend the GridSecurityProcessor, there isn't this problem: > > > > we have the explicit contract and can make default implementation > that > > > > throws an unsupported operation exception to enforcing compatibility > > > > check. > > > > > > > > In any case, we need to change GridSecurityProcessor implementation. > > > > > > > > But I think your proposal to try to find a security context in the > > > > node's attributes first is right for backward compatibility when > > > > Ignite users don't use thin clients. > > > > > > > > Summary: > > > > I suggest adding a new method to GridSecurityProcessor because it has > > > > a clear contract and enforces compatibility check natural way. > > > > > > > > вс, 15 мар. 2020 г. в 17:13, Alexei Scherbakov < > > > > [hidden email] > > > > >: > > > > > > > > > Denis Garus, > > > > > > > > > > I've looked at the IEP proposed by you and currently I'm thinking > > > > > it's > > > > not > > > > > immediately required. > > > > > > > > > > The problem of missing SecurityContexts of thin clients can be > > > > > solved > > > > much > > > > > easily. > > > > > > > > > > Below is the stub of a fix, it requires correct implementation of > > > > > method > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > > > #authenticatedSubject > > > > > by GridSecurityProcessor: > > > > > > > > > > /** {@inheritDoc} */ > > > > > @Override public OperationSecurityContext withContext(UUID > > nodeId) > > > { > > > > > try { > > > > > SecurityContext ctx0 = secCtxs.get(nodeId); > > > > > > > > > > if (ctx0 == null) { > > > > > ClusterNode node = > > > > > Optional.ofNullable(ctx.discovery().node(nodeId)) > > > > > .orElseGet(() -> > > > > > ctx.discovery().historicalNode(nodeId)); > > > > > > > > > > // This is a cluster node. > > > > > if (node != null) > > > > > ctx0 = nodeSecurityContext(marsh, > > > > > U.resolveClassLoader(ctx.config()), findNode(nodeId)); > > > > > else { > > > > > // This is already authenticated thin client. > > > > > SecuritySubject subj = > > > > > authenticatedSubject(nodeId); > > > > > > > > > > assert subj != null : "Subject is null " + > > > > > nodeId; > > > > > > > > > > AuthenticationContext actx = new > > > > > AuthenticationContext(); > > > > > actx.subject(subj); > > > > > > > > > > ctx0 = secPrc.authenticate(actx); > > > > > } > > > > > } > > > > > > > > > > secCtxs.putIfAbsent(nodeId, ctx0); > > > > > > > > > > return withContext(ctx0); > > > > > } catch (IgniteCheckedException e) { > > > > > throw new IgniteException(e); > > > > > } > > > > > > > > > > The idea is to create a thin client SecurityContext on a node not > > > > > having > > > > a > > > > > local context using existing SecuritySubject data. > > > > > > > > > > Method > > > > > > > > > > org.apache.ignite.internal.processors.security.GridSecurityProcessor#a > > > > uthenticate > > > > > should check for not null SecuritySubject field and just recreate > > > > > SecurityContext using passed info (because it's already > > authenticated). > > > > > > > > > > We have all necessary information in SecuritySubject returned by > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > > > #authenticatedSubject > > > > > by GridSecurityProcessor method. > > > > > > > > > > Because it is internal API, we may not care about compatibility at > > > > > all, but nevertheless it is possible to add compatibility check in > > > > > the method above. If a feature is not supported the operations from > > > > > thin clients should be forbidden. > > > > > > > > > > You proposal has the similar problem: if GridSecurityProcessor does > > > > > not support retriving context for thin clients, such clients will > > > > > not be able to proceed with operation. > > > > > > > > > > Still, the cleanup of security API is necessary and should be done > > > > > in 3.0 > > > > > > > > > > > > > > > > > > > > чт, 12 мар. 2020 г. в 16:48, VeenaMithare < > [hidden email] > > >: > > > > > > > > > > > HI , > > > > > > > > > > > > Created this jira : > > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > > > > > > > > > regards, > > > > > > Veena. > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Sent from: > http://apache-ignite-developers.2346864.n4.nabble.com/ > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > Best regards, > > > > > Alexei Scherbakov > > > > > > > > > > > > > > > > > > -- > > > > > > Best regards, > > > Alexei Scherbakov > > > ________________________________ > > > > > > > > > Spread bets and CFDs are complex instruments and come with a high risk > of > > > losing money rapidly due to leverage. 70.5% of retail investor accounts > > > lose money when spread betting and/or trading CFDs with this provider. > > You > > > should consider whether you understand how spread bets and CFDs work > and > > > whether you can afford to take the high risk of losing your money. > > > > > > Professional clients: Losses can exceed deposits when spread betting > and > > > trading CFDs. Countdowns carry a level of risk to your capital as you > > could > > > lose all of your investment. These products may not be suitable for all > > > clients therefore ensure you understand the risks and seek independent > > > advice. Invest only what you can afford to lose. > > > > > > CMC Markets UK plc (173730) and CMC Spreadbet plc (170627) are > authorised > > > and regulated by the Financial Conduct Authority in the United Kingdom. > > CMC > > > Markets UK plc and CMC Spreadbet plc are registered in England and > Wales > > > with Company Numbers 02448409 and 02589529 and with their registered > > > offices at 133 Houndsditch, London, EC3A 7BX. > > > > > > The content of this e-mail (including any attachments) is strictly > > > confidential and is for the sole use of the intended addressee(s). If > you > > > are not the intended recipient of this e-mail please notify the sender > > > immediately and delete this e-mail from your system. Any disclosure, > > > copying, dissemination or use of its content (including any > attachments) > > is > > > strictly prohibited. CMC Markets UK plc and CMC Spreadbet plc reserve > the > > > right to intercept and monitor the content of the e-mail messages to > and > > > from its systems. > > > > > > E-mails may be interfered with or may contain viruses or other defects > > for > > > which CMC Markets UK plc and CMC Spreadbet plc accept no > responsibility. > > It > > > is the responsibility of the recipient to carry out a virus check on > the > > > e-mail and any attachment(s). > > > > > > This communication is not intended as an offer or solicitation for the > > > purchase or sale of a financial instrument or as an official > confirmation > > > of any transaction unless specifically presented as such. > > > > > > CMC Markets UK plc and CMC Spreadbet plc are registered in England and > > > Wales with Company Numbers 02448409 and 02589529 and with their > > registered > > > offices at 133 Houndsditch, London, EC3A 7BX. > > > > > > -- Best regards, Alexei Scherbakov |
Alexey,
That can be another version of our plan. If everyone agrees that SecurityContext and SecuritySubject should be merged, such fix of thin clients' issue will bring us closer to the final solution. Denis, what do you think? On Tue, Mar 24, 2020 at 10:38 AM Alexei Scherbakov < [hidden email]> wrote: > Why can't we start gradually changing security API right now ? > I see no point in delaying with. > All changes will go to next 2.9 release anyway. > > My proposal: > 1. Get rid of security context. Doing this will bring security API to more > or less consistent state. > 2. Remove IEP-41 because it's no longer needed because of change [1] > 3. Propose an IEP to make security API avoid using internals. > > > > пн, 23 мар. 2020 г. в 19:53, Denis Garus <[hidden email]>: > > > Hello, Alexei, Ivan! > > > > >> Seems like security API is indeed a bit over-engineered > > > > Nobody has doubt we should do a reworking of GridSecurityProcessor. > > But this point is outside of scope thin client's problem that we are > > solving. > > I think we can create new IEP that will accumulate all ideas of Ignite's > > security improvements. > > > > >> Presence of the separate #securityContext(UUID) highlights that user > > indeed should care > > >> about propagation of thin clients' contexts between the cluster nodes. > > > > I agree with Ivan. I've implemented both variants, > > and I like one with #securityContext(UUID) more. > > > > Could you please take a look at PR [1] for the issue [2]? > > > > 1. https://github.com/apache/ignite/pull/7523 > > 2. https://issues.apache.org/jira/browse/IGNITE-12759 > > > > пн, 23 мар. 2020 г. в 11:45, Ivan Rakov <[hidden email]>: > > > > > Alex, Denis, > > > > > > Seems like security API is indeed a bit over-engineered. > > > > > > Let's get rid of SecurityContext and use SecuritySubject instead. > > > > SecurityContext is just a POJO wrapper over > > > > SecuritySubject's > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > > > > It's functionality can be easily moved to SecuritySubject. > > > > > > I totally agree. Both subject and context are implemented by plugin > > > provider, and I don't see any reason to keep both abstractions, > > especially > > > if we are going to get rid of transferring subject in node attributes > > > (argument that subject is more lightweight won't work anymore). > > > > > > Also, there's kind of mess in node authentication logic. There are at > > least > > > two components responsible for it: DiscoverySpiNodeAuthenticator (which > > is > > > forcibly set by GridDiscoveryManager, but in fact public) and > > > GridSecurityProcessor (which performs actual node auth logic, but > > private). > > > I also don't understand why we need both > > > #authenticate(AuthenticationContext) and #authenticateNode(ClusterNode, > > > SecurityCredentials) methods while it's possible to set explicit > > > SecuritySubjectType.REMOTE_NODE in AuthenticationContext (this is > > arguable; > > > perhaps there are strong reasons). > > > > > > Finally, areas of responsibility between IgniteSecurity and > > > GridSecurityProcessor are kind of mixed. As far as I understand, the > > first > > > is responsible for Ignite-internal management of security logic > (keeping > > > thread-local context, caching security contexts, etc; we don't expect > > > IgniteSecurity to be replaced by plugin provider) and the latter is > > > responsible for user-custom authentication / authorization logic. To be > > > honest, it took plenty of time to figure this out for me. > > > > > > From my point of view, we should make GridSecurityProcessor interface > > > public, rename it (it requires plenty of time to find the difference > from > > > IgniteSecurity), make its API as simple and non-duplicating as possible > > and > > > clarify its area of responsibility (e.g. should it be responsible for > > > propagation of successfully authenticated subject among all nodes or > > not?) > > > to make it easy to embed custom security logic in Ignite. > > > > > > Regarding thin clients fix: implementation made by Denis suits better > to > > > the very implicit contract that it's better to change API contracts of > an > > > internal IgniteSecurity than of internal GridSecurityProcessor (which > > > actually mustn't be internal). > > > > > > > My approach doesn't require any IEPs, just minor change in code and > to > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > > > > contract. > > > > > > Looks like a misuse of #authenticate method to me. It should perform > > > initial authentication based on credentials (this may include queries > to > > > external authentication subsystem, e.g. LDAP). User may want to don't > > > authenticate thin client on every node (this will increase the number > of > > > requests to auth subsystem unless user implicitly implements > propagation > > of > > > thin clients' contexts between nodes and make #authenticate > cluster-wide > > > idempotent: first call should perform actual authentication, next calls > > > should retrieve context of already authenticated client). Presence of > the > > > separate #securityContext(UUID) highlights that user indeed should care > > > about propagation of thin clients' contexts between the cluster nodes. > > > > > > -- > > > Ivan > > > > > > On Fri, Mar 20, 2020 at 12:22 PM Veena Mithare < > [hidden email] > > > > > > wrote: > > > > > > > Hi Alexei, Denis, > > > > > > > > One of the main usecases of thin client authentication is to be able > to > > > > audit the changes done using the thin client user. > > > > To enable that : > > > > We really need to resolve this concern as well : > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > > > > > ( Incorrect security subject id is associated with a cache_put event > > > > when the originator of the event is a thin client. ) > > > > > > > > Regards, > > > > Veena > > > > > > > > > > > > -----Original Message----- > > > > From: Alexei Scherbakov <[hidden email]> > > > > Sent: 18 March 2020 08:11 > > > > To: dev <[hidden email]> > > > > Subject: Re: Security Subject of thin client on remote nodes > > > > > > > > Denis Garus, > > > > > > > > Both variants are capable of solving the thin client security context > > > > problem. > > > > > > > > My approach doesn't require any IEPs, just minor change in code and > to > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > > > > contract. > > > > We can add appropriate documentation to emphasize this. > > > > The argument "fragile" is not very convincing for me. > > > > > > > > I think we should collect more opinions before proceeding with IEP. > > > > > > > > Considering a fact we actually *may not care* about compatibility > (I've > > > > already explained why), I'm thinking of another approach. > > > > Let's get rid of SecurityContext and use SecuritySubject instead. > > > > SecurityContext is just a POJO wrapper over SecuritySubject's > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > > > > It's functionality can be easily moved to SecuritySubject. > > > > > > > > What do you think? > > > > > > > > > > > > > > > > пн, 16 мар. 2020 г. в 15:47, Denis Garus <[hidden email]>: > > > > > > > > > Hello, Alexei! > > > > > > > > > > I agree with you if we may not care about compatibility at all, > then > > > > > we can solve the problem much more straightforward way. > > > > > > > > > > In your case, the method GridSecurityProcessor#authenticate will > have > > > > > an implicit contract: > > > > > [ if actx.subject() != null then > > > > > returns SecurityContext > > > > > else > > > > > do authenticate ] > > > > > > > > > > It looks fragile. > > > > > > > > > > When we extend the GridSecurityProcessor, there isn't this problem: > > > > > we have the explicit contract and can make default implementation > > that > > > > > throws an unsupported operation exception to enforcing > compatibility > > > > > check. > > > > > > > > > > In any case, we need to change GridSecurityProcessor > implementation. > > > > > > > > > > But I think your proposal to try to find a security context in the > > > > > node's attributes first is right for backward compatibility when > > > > > Ignite users don't use thin clients. > > > > > > > > > > Summary: > > > > > I suggest adding a new method to GridSecurityProcessor because it > has > > > > > a clear contract and enforces compatibility check natural way. > > > > > > > > > > вс, 15 мар. 2020 г. в 17:13, Alexei Scherbakov < > > > > > [hidden email] > > > > > >: > > > > > > > > > > > Denis Garus, > > > > > > > > > > > > I've looked at the IEP proposed by you and currently I'm thinking > > > > > > it's > > > > > not > > > > > > immediately required. > > > > > > > > > > > > The problem of missing SecurityContexts of thin clients can be > > > > > > solved > > > > > much > > > > > > easily. > > > > > > > > > > > > Below is the stub of a fix, it requires correct implementation of > > > > > > method > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > > > > #authenticatedSubject > > > > > > by GridSecurityProcessor: > > > > > > > > > > > > /** {@inheritDoc} */ > > > > > > @Override public OperationSecurityContext withContext(UUID > > > nodeId) > > > > { > > > > > > try { > > > > > > SecurityContext ctx0 = secCtxs.get(nodeId); > > > > > > > > > > > > if (ctx0 == null) { > > > > > > ClusterNode node = > > > > > > Optional.ofNullable(ctx.discovery().node(nodeId)) > > > > > > .orElseGet(() -> > > > > > > ctx.discovery().historicalNode(nodeId)); > > > > > > > > > > > > // This is a cluster node. > > > > > > if (node != null) > > > > > > ctx0 = nodeSecurityContext(marsh, > > > > > > U.resolveClassLoader(ctx.config()), findNode(nodeId)); > > > > > > else { > > > > > > // This is already authenticated thin client. > > > > > > SecuritySubject subj = > > > > > > authenticatedSubject(nodeId); > > > > > > > > > > > > assert subj != null : "Subject is null " + > > > > > > nodeId; > > > > > > > > > > > > AuthenticationContext actx = new > > > > > > AuthenticationContext(); > > > > > > actx.subject(subj); > > > > > > > > > > > > ctx0 = secPrc.authenticate(actx); > > > > > > } > > > > > > } > > > > > > > > > > > > secCtxs.putIfAbsent(nodeId, ctx0); > > > > > > > > > > > > return withContext(ctx0); > > > > > > } catch (IgniteCheckedException e) { > > > > > > throw new IgniteException(e); > > > > > > } > > > > > > > > > > > > The idea is to create a thin client SecurityContext on a node not > > > > > > having > > > > > a > > > > > > local context using existing SecuritySubject data. > > > > > > > > > > > > Method > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.GridSecurityProcessor#a > > > > > uthenticate > > > > > > should check for not null SecuritySubject field and just recreate > > > > > > SecurityContext using passed info (because it's already > > > authenticated). > > > > > > > > > > > > We have all necessary information in SecuritySubject returned by > > > > > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > > > > #authenticatedSubject > > > > > > by GridSecurityProcessor method. > > > > > > > > > > > > Because it is internal API, we may not care about compatibility > at > > > > > > all, but nevertheless it is possible to add compatibility check > in > > > > > > the method above. If a feature is not supported the operations > from > > > > > > thin clients should be forbidden. > > > > > > > > > > > > You proposal has the similar problem: if GridSecurityProcessor > does > > > > > > not support retriving context for thin clients, such clients will > > > > > > not be able to proceed with operation. > > > > > > > > > > > > Still, the cleanup of security API is necessary and should be > done > > > > > > in 3.0 > > > > > > > > > > > > > > > > > > > > > > > > чт, 12 мар. 2020 г. в 16:48, VeenaMithare < > > [hidden email] > > > >: > > > > > > > > > > > > > HI , > > > > > > > > > > > > > > Created this jira : > > > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > > > > > > > > > > > regards, > > > > > > > Veena. > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > Sent from: > > http://apache-ignite-developers.2346864.n4.nabble.com/ > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > Best regards, > > > > > > Alexei Scherbakov > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > Best regards, > > > > Alexei Scherbakov > > > > ________________________________ > > > > > > > > > > > > Spread bets and CFDs are complex instruments and come with a high > risk > > of > > > > losing money rapidly due to leverage. 70.5% of retail investor > accounts > > > > lose money when spread betting and/or trading CFDs with this > provider. > > > You > > > > should consider whether you understand how spread bets and CFDs work > > and > > > > whether you can afford to take the high risk of losing your money. > > > > > > > > Professional clients: Losses can exceed deposits when spread betting > > and > > > > trading CFDs. Countdowns carry a level of risk to your capital as you > > > could > > > > lose all of your investment. These products may not be suitable for > all > > > > clients therefore ensure you understand the risks and seek > independent > > > > advice. Invest only what you can afford to lose. > > > > > > > > CMC Markets UK plc (173730) and CMC Spreadbet plc (170627) are > > authorised > > > > and regulated by the Financial Conduct Authority in the United > Kingdom. > > > CMC > > > > Markets UK plc and CMC Spreadbet plc are registered in England and > > Wales > > > > with Company Numbers 02448409 and 02589529 and with their registered > > > > offices at 133 Houndsditch, London, EC3A 7BX. > > > > > > > > The content of this e-mail (including any attachments) is strictly > > > > confidential and is for the sole use of the intended addressee(s). If > > you > > > > are not the intended recipient of this e-mail please notify the > sender > > > > immediately and delete this e-mail from your system. Any disclosure, > > > > copying, dissemination or use of its content (including any > > attachments) > > > is > > > > strictly prohibited. CMC Markets UK plc and CMC Spreadbet plc reserve > > the > > > > right to intercept and monitor the content of the e-mail messages to > > and > > > > from its systems. > > > > > > > > E-mails may be interfered with or may contain viruses or other > defects > > > for > > > > which CMC Markets UK plc and CMC Spreadbet plc accept no > > responsibility. > > > It > > > > is the responsibility of the recipient to carry out a virus check on > > the > > > > e-mail and any attachment(s). > > > > > > > > This communication is not intended as an offer or solicitation for > the > > > > purchase or sale of a financial instrument or as an official > > confirmation > > > > of any transaction unless specifically presented as such. > > > > > > > > CMC Markets UK plc and CMC Spreadbet plc are registered in England > and > > > > Wales with Company Numbers 02448409 and 02589529 and with their > > > registered > > > > offices at 133 Houndsditch, London, EC3A 7BX. > > > > > > > > > > > > -- > > Best regards, > Alexei Scherbakov > |
Hi, guys!
I agree that we should rework the security API, but it can take a long time. And currently, our users have certain impediments that are blockers for their job. I think we have to fix bugs that IEP-41 [1] contains as soon as possible to support our users. From my point of view, IEP-41 is the best place to track bug fixing. 1. https://cwiki.apache.org/confluence/display/IGNITE/IEP-41%3A+Security+Context+of+thin+client+on+remote+nodes вт, 24 мар. 2020 г. в 12:26, Ivan Rakov <[hidden email]>: > Alexey, > > That can be another version of our plan. If everyone agrees that > SecurityContext and SecuritySubject should be merged, such fix of thin > clients' issue will bring us closer to the final solution. > Denis, what do you think? > > On Tue, Mar 24, 2020 at 10:38 AM Alexei Scherbakov < > [hidden email]> wrote: > > > Why can't we start gradually changing security API right now ? > > I see no point in delaying with. > > All changes will go to next 2.9 release anyway. > > > > My proposal: > > 1. Get rid of security context. Doing this will bring security API to > more > > or less consistent state. > > 2. Remove IEP-41 because it's no longer needed because of change [1] > > 3. Propose an IEP to make security API avoid using internals. > > > > > > > > пн, 23 мар. 2020 г. в 19:53, Denis Garus <[hidden email]>: > > > > > Hello, Alexei, Ivan! > > > > > > >> Seems like security API is indeed a bit over-engineered > > > > > > Nobody has doubt we should do a reworking of GridSecurityProcessor. > > > But this point is outside of scope thin client's problem that we are > > > solving. > > > I think we can create new IEP that will accumulate all ideas of > Ignite's > > > security improvements. > > > > > > >> Presence of the separate #securityContext(UUID) highlights that user > > > indeed should care > > > >> about propagation of thin clients' contexts between the cluster > nodes. > > > > > > I agree with Ivan. I've implemented both variants, > > > and I like one with #securityContext(UUID) more. > > > > > > Could you please take a look at PR [1] for the issue [2]? > > > > > > 1. https://github.com/apache/ignite/pull/7523 > > > 2. https://issues.apache.org/jira/browse/IGNITE-12759 > > > > > > пн, 23 мар. 2020 г. в 11:45, Ivan Rakov <[hidden email]>: > > > > > > > Alex, Denis, > > > > > > > > Seems like security API is indeed a bit over-engineered. > > > > > > > > Let's get rid of SecurityContext and use SecuritySubject instead. > > > > > SecurityContext is just a POJO wrapper over > > > > > SecuritySubject's > > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > > > > > It's functionality can be easily moved to SecuritySubject. > > > > > > > > I totally agree. Both subject and context are implemented by plugin > > > > provider, and I don't see any reason to keep both abstractions, > > > especially > > > > if we are going to get rid of transferring subject in node attributes > > > > (argument that subject is more lightweight won't work anymore). > > > > > > > > Also, there's kind of mess in node authentication logic. There are at > > > least > > > > two components responsible for it: DiscoverySpiNodeAuthenticator > (which > > > is > > > > forcibly set by GridDiscoveryManager, but in fact public) and > > > > GridSecurityProcessor (which performs actual node auth logic, but > > > private). > > > > I also don't understand why we need both > > > > #authenticate(AuthenticationContext) and > #authenticateNode(ClusterNode, > > > > SecurityCredentials) methods while it's possible to set explicit > > > > SecuritySubjectType.REMOTE_NODE in AuthenticationContext (this is > > > arguable; > > > > perhaps there are strong reasons). > > > > > > > > Finally, areas of responsibility between IgniteSecurity and > > > > GridSecurityProcessor are kind of mixed. As far as I understand, the > > > first > > > > is responsible for Ignite-internal management of security logic > > (keeping > > > > thread-local context, caching security contexts, etc; we don't expect > > > > IgniteSecurity to be replaced by plugin provider) and the latter is > > > > responsible for user-custom authentication / authorization logic. To > be > > > > honest, it took plenty of time to figure this out for me. > > > > > > > > From my point of view, we should make GridSecurityProcessor interface > > > > public, rename it (it requires plenty of time to find the difference > > from > > > > IgniteSecurity), make its API as simple and non-duplicating as > possible > > > and > > > > clarify its area of responsibility (e.g. should it be responsible for > > > > propagation of successfully authenticated subject among all nodes or > > > not?) > > > > to make it easy to embed custom security logic in Ignite. > > > > > > > > Regarding thin clients fix: implementation made by Denis suits better > > to > > > > the very implicit contract that it's better to change API contracts > of > > an > > > > internal IgniteSecurity than of internal GridSecurityProcessor (which > > > > actually mustn't be internal). > > > > > > > > > My approach doesn't require any IEPs, just minor change in code and > > to > > > > > > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > > > > > contract. > > > > > > > > Looks like a misuse of #authenticate method to me. It should perform > > > > initial authentication based on credentials (this may include queries > > to > > > > external authentication subsystem, e.g. LDAP). User may want to don't > > > > authenticate thin client on every node (this will increase the number > > of > > > > requests to auth subsystem unless user implicitly implements > > propagation > > > of > > > > thin clients' contexts between nodes and make #authenticate > > cluster-wide > > > > idempotent: first call should perform actual authentication, next > calls > > > > should retrieve context of already authenticated client). Presence of > > the > > > > separate #securityContext(UUID) highlights that user indeed should > care > > > > about propagation of thin clients' contexts between the cluster > nodes. > > > > > > > > -- > > > > Ivan > > > > > > > > On Fri, Mar 20, 2020 at 12:22 PM Veena Mithare < > > [hidden email] > > > > > > > > wrote: > > > > > > > > > Hi Alexei, Denis, > > > > > > > > > > One of the main usecases of thin client authentication is to be > able > > to > > > > > audit the changes done using the thin client user. > > > > > To enable that : > > > > > We really need to resolve this concern as well : > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > > > > > > > ( Incorrect security subject id is associated with a cache_put > event > > > > > when the originator of the event is a thin client. ) > > > > > > > > > > Regards, > > > > > Veena > > > > > > > > > > > > > > > -----Original Message----- > > > > > From: Alexei Scherbakov <[hidden email]> > > > > > Sent: 18 March 2020 08:11 > > > > > To: dev <[hidden email]> > > > > > Subject: Re: Security Subject of thin client on remote nodes > > > > > > > > > > Denis Garus, > > > > > > > > > > Both variants are capable of solving the thin client security > context > > > > > problem. > > > > > > > > > > My approach doesn't require any IEPs, just minor change in code and > > to > > > > > > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > > > > > contract. > > > > > We can add appropriate documentation to emphasize this. > > > > > The argument "fragile" is not very convincing for me. > > > > > > > > > > I think we should collect more opinions before proceeding with IEP. > > > > > > > > > > Considering a fact we actually *may not care* about compatibility > > (I've > > > > > already explained why), I'm thinking of another approach. > > > > > Let's get rid of SecurityContext and use SecuritySubject instead. > > > > > SecurityContext is just a POJO wrapper over SecuritySubject's > > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > > > > > It's functionality can be easily moved to SecuritySubject. > > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > > > > > пн, 16 мар. 2020 г. в 15:47, Denis Garus <[hidden email]>: > > > > > > > > > > > Hello, Alexei! > > > > > > > > > > > > I agree with you if we may not care about compatibility at all, > > then > > > > > > we can solve the problem much more straightforward way. > > > > > > > > > > > > In your case, the method GridSecurityProcessor#authenticate will > > have > > > > > > an implicit contract: > > > > > > [ if actx.subject() != null then > > > > > > returns SecurityContext > > > > > > else > > > > > > do authenticate ] > > > > > > > > > > > > It looks fragile. > > > > > > > > > > > > When we extend the GridSecurityProcessor, there isn't this > problem: > > > > > > we have the explicit contract and can make default implementation > > > that > > > > > > throws an unsupported operation exception to enforcing > > compatibility > > > > > > check. > > > > > > > > > > > > In any case, we need to change GridSecurityProcessor > > implementation. > > > > > > > > > > > > But I think your proposal to try to find a security context in > the > > > > > > node's attributes first is right for backward compatibility when > > > > > > Ignite users don't use thin clients. > > > > > > > > > > > > Summary: > > > > > > I suggest adding a new method to GridSecurityProcessor because it > > has > > > > > > a clear contract and enforces compatibility check natural way. > > > > > > > > > > > > вс, 15 мар. 2020 г. в 17:13, Alexei Scherbakov < > > > > > > [hidden email] > > > > > > >: > > > > > > > > > > > > > Denis Garus, > > > > > > > > > > > > > > I've looked at the IEP proposed by you and currently I'm > thinking > > > > > > > it's > > > > > > not > > > > > > > immediately required. > > > > > > > > > > > > > > The problem of missing SecurityContexts of thin clients can be > > > > > > > solved > > > > > > much > > > > > > > easily. > > > > > > > > > > > > > > Below is the stub of a fix, it requires correct implementation > of > > > > > > > method > > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > > > > > #authenticatedSubject > > > > > > > by GridSecurityProcessor: > > > > > > > > > > > > > > /** {@inheritDoc} */ > > > > > > > @Override public OperationSecurityContext withContext(UUID > > > > nodeId) > > > > > { > > > > > > > try { > > > > > > > SecurityContext ctx0 = secCtxs.get(nodeId); > > > > > > > > > > > > > > if (ctx0 == null) { > > > > > > > ClusterNode node = > > > > > > > Optional.ofNullable(ctx.discovery().node(nodeId)) > > > > > > > .orElseGet(() -> > > > > > > > ctx.discovery().historicalNode(nodeId)); > > > > > > > > > > > > > > // This is a cluster node. > > > > > > > if (node != null) > > > > > > > ctx0 = nodeSecurityContext(marsh, > > > > > > > U.resolveClassLoader(ctx.config()), findNode(nodeId)); > > > > > > > else { > > > > > > > // This is already authenticated thin > client. > > > > > > > SecuritySubject subj = > > > > > > > authenticatedSubject(nodeId); > > > > > > > > > > > > > > assert subj != null : "Subject is null " + > > > > > > > nodeId; > > > > > > > > > > > > > > AuthenticationContext actx = new > > > > > > > AuthenticationContext(); > > > > > > > actx.subject(subj); > > > > > > > > > > > > > > ctx0 = secPrc.authenticate(actx); > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > secCtxs.putIfAbsent(nodeId, ctx0); > > > > > > > > > > > > > > return withContext(ctx0); > > > > > > > } catch (IgniteCheckedException e) { > > > > > > > throw new IgniteException(e); > > > > > > > } > > > > > > > > > > > > > > The idea is to create a thin client SecurityContext on a node > not > > > > > > > having > > > > > > a > > > > > > > local context using existing SecuritySubject data. > > > > > > > > > > > > > > Method > > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.GridSecurityProcessor#a > > > > > > uthenticate > > > > > > > should check for not null SecuritySubject field and just > recreate > > > > > > > SecurityContext using passed info (because it's already > > > > authenticated). > > > > > > > > > > > > > > We have all necessary information in SecuritySubject returned > by > > > > > > > > > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > > > > > #authenticatedSubject > > > > > > > by GridSecurityProcessor method. > > > > > > > > > > > > > > Because it is internal API, we may not care about > compatibility > > at > > > > > > > all, but nevertheless it is possible to add compatibility check > > in > > > > > > > the method above. If a feature is not supported the operations > > from > > > > > > > thin clients should be forbidden. > > > > > > > > > > > > > > You proposal has the similar problem: if GridSecurityProcessor > > does > > > > > > > not support retriving context for thin clients, such clients > will > > > > > > > not be able to proceed with operation. > > > > > > > > > > > > > > Still, the cleanup of security API is necessary and should be > > done > > > > > > > in 3.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > чт, 12 мар. 2020 г. в 16:48, VeenaMithare < > > > [hidden email] > > > > >: > > > > > > > > > > > > > > > HI , > > > > > > > > > > > > > > > > Created this jira : > > > > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > > > > > > > > > > > > > regards, > > > > > > > > Veena. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > Sent from: > > > http://apache-ignite-developers.2346864.n4.nabble.com/ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > Best regards, > > > > > > > Alexei Scherbakov > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > Best regards, > > > > > Alexei Scherbakov > > > > > ________________________________ > > > > > > > > > > > > > > > Spread bets and CFDs are complex instruments and come with a high > > risk > > > of > > > > > losing money rapidly due to leverage. 70.5% of retail investor > > accounts > > > > > lose money when spread betting and/or trading CFDs with this > > provider. > > > > You > > > > > should consider whether you understand how spread bets and CFDs > work > > > and > > > > > whether you can afford to take the high risk of losing your money. > > > > > > > > > > Professional clients: Losses can exceed deposits when spread > betting > > > and > > > > > trading CFDs. Countdowns carry a level of risk to your capital as > you > > > > could > > > > > lose all of your investment. These products may not be suitable for > > all > > > > > clients therefore ensure you understand the risks and seek > > independent > > > > > advice. Invest only what you can afford to lose. > > > > > > > > > > CMC Markets UK plc (173730) and CMC Spreadbet plc (170627) are > > > authorised > > > > > and regulated by the Financial Conduct Authority in the United > > Kingdom. > > > > CMC > > > > > Markets UK plc and CMC Spreadbet plc are registered in England and > > > Wales > > > > > with Company Numbers 02448409 and 02589529 and with their > registered > > > > > offices at 133 Houndsditch, London, EC3A 7BX. > > > > > > > > > > The content of this e-mail (including any attachments) is strictly > > > > > confidential and is for the sole use of the intended addressee(s). > If > > > you > > > > > are not the intended recipient of this e-mail please notify the > > sender > > > > > immediately and delete this e-mail from your system. Any > disclosure, > > > > > copying, dissemination or use of its content (including any > > > attachments) > > > > is > > > > > strictly prohibited. CMC Markets UK plc and CMC Spreadbet plc > reserve > > > the > > > > > right to intercept and monitor the content of the e-mail messages > to > > > and > > > > > from its systems. > > > > > > > > > > E-mails may be interfered with or may contain viruses or other > > defects > > > > for > > > > > which CMC Markets UK plc and CMC Spreadbet plc accept no > > > responsibility. > > > > It > > > > > is the responsibility of the recipient to carry out a virus check > on > > > the > > > > > e-mail and any attachment(s). > > > > > > > > > > This communication is not intended as an offer or solicitation for > > the > > > > > purchase or sale of a financial instrument or as an official > > > confirmation > > > > > of any transaction unless specifically presented as such. > > > > > > > > > > CMC Markets UK plc and CMC Spreadbet plc are registered in England > > and > > > > > Wales with Company Numbers 02448409 and 02589529 and with their > > > > registered > > > > > offices at 133 Houndsditch, London, EC3A 7BX. > > > > > > > > > > > > > > > > > > -- > > > > Best regards, > > Alexei Scherbakov > > > |
Denis,
I'll review your PR. If this issue is a subject to be included in 2.8.1 in emergency mode, I'm ok with the current API changes. Please think about driving creating IEP on security API overhaul prior to 2.9. I believe that you are the most suitable Ignite community member to drive this activity. I'd love to share some ideas as well. On Tue, Mar 24, 2020 at 2:04 PM Denis Garus <[hidden email]> wrote: > Hi, guys! > > > I agree that we should rework the security API, but it can take a long > time. > > And currently, our users have certain impediments that are blockers for > their job. > > I think we have to fix bugs that IEP-41 [1] contains as soon as possible to > support our users. > > From my point of view, IEP-41 is the best place to track bug fixing. > > > > 1. > > https://cwiki.apache.org/confluence/display/IGNITE/IEP-41%3A+Security+Context+of+thin+client+on+remote+nodes > > > вт, 24 мар. 2020 г. в 12:26, Ivan Rakov <[hidden email]>: > > > Alexey, > > > > That can be another version of our plan. If everyone agrees that > > SecurityContext and SecuritySubject should be merged, such fix of thin > > clients' issue will bring us closer to the final solution. > > Denis, what do you think? > > > > On Tue, Mar 24, 2020 at 10:38 AM Alexei Scherbakov < > > [hidden email]> wrote: > > > > > Why can't we start gradually changing security API right now ? > > > I see no point in delaying with. > > > All changes will go to next 2.9 release anyway. > > > > > > My proposal: > > > 1. Get rid of security context. Doing this will bring security API to > > more > > > or less consistent state. > > > 2. Remove IEP-41 because it's no longer needed because of change [1] > > > 3. Propose an IEP to make security API avoid using internals. > > > > > > > > > > > > пн, 23 мар. 2020 г. в 19:53, Denis Garus <[hidden email]>: > > > > > > > Hello, Alexei, Ivan! > > > > > > > > >> Seems like security API is indeed a bit over-engineered > > > > > > > > Nobody has doubt we should do a reworking of GridSecurityProcessor. > > > > But this point is outside of scope thin client's problem that we are > > > > solving. > > > > I think we can create new IEP that will accumulate all ideas of > > Ignite's > > > > security improvements. > > > > > > > > >> Presence of the separate #securityContext(UUID) highlights that > user > > > > indeed should care > > > > >> about propagation of thin clients' contexts between the cluster > > nodes. > > > > > > > > I agree with Ivan. I've implemented both variants, > > > > and I like one with #securityContext(UUID) more. > > > > > > > > Could you please take a look at PR [1] for the issue [2]? > > > > > > > > 1. https://github.com/apache/ignite/pull/7523 > > > > 2. https://issues.apache.org/jira/browse/IGNITE-12759 > > > > > > > > пн, 23 мар. 2020 г. в 11:45, Ivan Rakov <[hidden email]>: > > > > > > > > > Alex, Denis, > > > > > > > > > > Seems like security API is indeed a bit over-engineered. > > > > > > > > > > Let's get rid of SecurityContext and use SecuritySubject instead. > > > > > > SecurityContext is just a POJO wrapper over > > > > > > SecuritySubject's > > > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > > > > > > It's functionality can be easily moved to SecuritySubject. > > > > > > > > > > I totally agree. Both subject and context are implemented by plugin > > > > > provider, and I don't see any reason to keep both abstractions, > > > > especially > > > > > if we are going to get rid of transferring subject in node > attributes > > > > > (argument that subject is more lightweight won't work anymore). > > > > > > > > > > Also, there's kind of mess in node authentication logic. There are > at > > > > least > > > > > two components responsible for it: DiscoverySpiNodeAuthenticator > > (which > > > > is > > > > > forcibly set by GridDiscoveryManager, but in fact public) and > > > > > GridSecurityProcessor (which performs actual node auth logic, but > > > > private). > > > > > I also don't understand why we need both > > > > > #authenticate(AuthenticationContext) and > > #authenticateNode(ClusterNode, > > > > > SecurityCredentials) methods while it's possible to set explicit > > > > > SecuritySubjectType.REMOTE_NODE in AuthenticationContext (this is > > > > arguable; > > > > > perhaps there are strong reasons). > > > > > > > > > > Finally, areas of responsibility between IgniteSecurity and > > > > > GridSecurityProcessor are kind of mixed. As far as I understand, > the > > > > first > > > > > is responsible for Ignite-internal management of security logic > > > (keeping > > > > > thread-local context, caching security contexts, etc; we don't > expect > > > > > IgniteSecurity to be replaced by plugin provider) and the latter is > > > > > responsible for user-custom authentication / authorization logic. > To > > be > > > > > honest, it took plenty of time to figure this out for me. > > > > > > > > > > From my point of view, we should make GridSecurityProcessor > interface > > > > > public, rename it (it requires plenty of time to find the > difference > > > from > > > > > IgniteSecurity), make its API as simple and non-duplicating as > > possible > > > > and > > > > > clarify its area of responsibility (e.g. should it be responsible > for > > > > > propagation of successfully authenticated subject among all nodes > or > > > > not?) > > > > > to make it easy to embed custom security logic in Ignite. > > > > > > > > > > Regarding thin clients fix: implementation made by Denis suits > better > > > to > > > > > the very implicit contract that it's better to change API contracts > > of > > > an > > > > > internal IgniteSecurity than of internal GridSecurityProcessor > (which > > > > > actually mustn't be internal). > > > > > > > > > > > My approach doesn't require any IEPs, just minor change in code > and > > > to > > > > > > > > > > > > > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > > > > > > contract. > > > > > > > > > > Looks like a misuse of #authenticate method to me. It should > perform > > > > > initial authentication based on credentials (this may include > queries > > > to > > > > > external authentication subsystem, e.g. LDAP). User may want to > don't > > > > > authenticate thin client on every node (this will increase the > number > > > of > > > > > requests to auth subsystem unless user implicitly implements > > > propagation > > > > of > > > > > thin clients' contexts between nodes and make #authenticate > > > cluster-wide > > > > > idempotent: first call should perform actual authentication, next > > calls > > > > > should retrieve context of already authenticated client). Presence > of > > > the > > > > > separate #securityContext(UUID) highlights that user indeed should > > care > > > > > about propagation of thin clients' contexts between the cluster > > nodes. > > > > > > > > > > -- > > > > > Ivan > > > > > > > > > > On Fri, Mar 20, 2020 at 12:22 PM Veena Mithare < > > > [hidden email] > > > > > > > > > > wrote: > > > > > > > > > > > Hi Alexei, Denis, > > > > > > > > > > > > One of the main usecases of thin client authentication is to be > > able > > > to > > > > > > audit the changes done using the thin client user. > > > > > > To enable that : > > > > > > We really need to resolve this concern as well : > > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > > > > > > > > > ( Incorrect security subject id is associated with a cache_put > > event > > > > > > when the originator of the event is a thin client. ) > > > > > > > > > > > > Regards, > > > > > > Veena > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Alexei Scherbakov <[hidden email]> > > > > > > Sent: 18 March 2020 08:11 > > > > > > To: dev <[hidden email]> > > > > > > Subject: Re: Security Subject of thin client on remote nodes > > > > > > > > > > > > Denis Garus, > > > > > > > > > > > > Both variants are capable of solving the thin client security > > context > > > > > > problem. > > > > > > > > > > > > My approach doesn't require any IEPs, just minor change in code > and > > > to > > > > > > > > > > > > > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > > > > > > contract. > > > > > > We can add appropriate documentation to emphasize this. > > > > > > The argument "fragile" is not very convincing for me. > > > > > > > > > > > > I think we should collect more opinions before proceeding with > IEP. > > > > > > > > > > > > Considering a fact we actually *may not care* about compatibility > > > (I've > > > > > > already explained why), I'm thinking of another approach. > > > > > > Let's get rid of SecurityContext and use SecuritySubject instead. > > > > > > SecurityContext is just a POJO wrapper over SecuritySubject's > > > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > > > > > > It's functionality can be easily moved to SecuritySubject. > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > > > > > > > > > пн, 16 мар. 2020 г. в 15:47, Denis Garus <[hidden email]>: > > > > > > > > > > > > > Hello, Alexei! > > > > > > > > > > > > > > I agree with you if we may not care about compatibility at all, > > > then > > > > > > > we can solve the problem much more straightforward way. > > > > > > > > > > > > > > In your case, the method GridSecurityProcessor#authenticate > will > > > have > > > > > > > an implicit contract: > > > > > > > [ if actx.subject() != null then > > > > > > > returns SecurityContext > > > > > > > else > > > > > > > do authenticate ] > > > > > > > > > > > > > > It looks fragile. > > > > > > > > > > > > > > When we extend the GridSecurityProcessor, there isn't this > > problem: > > > > > > > we have the explicit contract and can make default > implementation > > > > that > > > > > > > throws an unsupported operation exception to enforcing > > > compatibility > > > > > > > check. > > > > > > > > > > > > > > In any case, we need to change GridSecurityProcessor > > > implementation. > > > > > > > > > > > > > > But I think your proposal to try to find a security context in > > the > > > > > > > node's attributes first is right for backward compatibility > when > > > > > > > Ignite users don't use thin clients. > > > > > > > > > > > > > > Summary: > > > > > > > I suggest adding a new method to GridSecurityProcessor because > it > > > has > > > > > > > a clear contract and enforces compatibility check natural way. > > > > > > > > > > > > > > вс, 15 мар. 2020 г. в 17:13, Alexei Scherbakov < > > > > > > > [hidden email] > > > > > > > >: > > > > > > > > > > > > > > > Denis Garus, > > > > > > > > > > > > > > > > I've looked at the IEP proposed by you and currently I'm > > thinking > > > > > > > > it's > > > > > > > not > > > > > > > > immediately required. > > > > > > > > > > > > > > > > The problem of missing SecurityContexts of thin clients can > be > > > > > > > > solved > > > > > > > much > > > > > > > > easily. > > > > > > > > > > > > > > > > Below is the stub of a fix, it requires correct > implementation > > of > > > > > > > > method > > > > > > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > > > > > > #authenticatedSubject > > > > > > > > by GridSecurityProcessor: > > > > > > > > > > > > > > > > /** {@inheritDoc} */ > > > > > > > > @Override public OperationSecurityContext > withContext(UUID > > > > > nodeId) > > > > > > { > > > > > > > > try { > > > > > > > > SecurityContext ctx0 = secCtxs.get(nodeId); > > > > > > > > > > > > > > > > if (ctx0 == null) { > > > > > > > > ClusterNode node = > > > > > > > > Optional.ofNullable(ctx.discovery().node(nodeId)) > > > > > > > > .orElseGet(() -> > > > > > > > > ctx.discovery().historicalNode(nodeId)); > > > > > > > > > > > > > > > > // This is a cluster node. > > > > > > > > if (node != null) > > > > > > > > ctx0 = nodeSecurityContext(marsh, > > > > > > > > U.resolveClassLoader(ctx.config()), findNode(nodeId)); > > > > > > > > else { > > > > > > > > // This is already authenticated thin > > client. > > > > > > > > SecuritySubject subj = > > > > > > > > authenticatedSubject(nodeId); > > > > > > > > > > > > > > > > assert subj != null : "Subject is null " > + > > > > > > > > nodeId; > > > > > > > > > > > > > > > > AuthenticationContext actx = new > > > > > > > > AuthenticationContext(); > > > > > > > > actx.subject(subj); > > > > > > > > > > > > > > > > ctx0 = secPrc.authenticate(actx); > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > > > secCtxs.putIfAbsent(nodeId, ctx0); > > > > > > > > > > > > > > > > return withContext(ctx0); > > > > > > > > } catch (IgniteCheckedException e) { > > > > > > > > throw new IgniteException(e); > > > > > > > > } > > > > > > > > > > > > > > > > The idea is to create a thin client SecurityContext on a node > > not > > > > > > > > having > > > > > > > a > > > > > > > > local context using existing SecuritySubject data. > > > > > > > > > > > > > > > > Method > > > > > > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.GridSecurityProcessor#a > > > > > > > uthenticate > > > > > > > > should check for not null SecuritySubject field and just > > recreate > > > > > > > > SecurityContext using passed info (because it's already > > > > > authenticated). > > > > > > > > > > > > > > > > We have all necessary information in SecuritySubject returned > > by > > > > > > > > > > > > > > > > > > > > > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > > > > > > #authenticatedSubject > > > > > > > > by GridSecurityProcessor method. > > > > > > > > > > > > > > > > Because it is internal API, we may not care about > > compatibility > > > at > > > > > > > > all, but nevertheless it is possible to add compatibility > check > > > in > > > > > > > > the method above. If a feature is not supported the > operations > > > from > > > > > > > > thin clients should be forbidden. > > > > > > > > > > > > > > > > You proposal has the similar problem: if > GridSecurityProcessor > > > does > > > > > > > > not support retriving context for thin clients, such clients > > will > > > > > > > > not be able to proceed with operation. > > > > > > > > > > > > > > > > Still, the cleanup of security API is necessary and should be > > > done > > > > > > > > in 3.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > чт, 12 мар. 2020 г. в 16:48, VeenaMithare < > > > > [hidden email] > > > > > >: > > > > > > > > > > > > > > > > > HI , > > > > > > > > > > > > > > > > > > Created this jira : > > > > > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > > > > > > > > > > > > > > > regards, > > > > > > > > > Veena. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > Sent from: > > > > http://apache-ignite-developers.2346864.n4.nabble.com/ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > > > Best regards, > > > > > > > > Alexei Scherbakov > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > Best regards, > > > > > > Alexei Scherbakov > > > > > > ________________________________ > > > > > > > > > > > > > > > > > > Spread bets and CFDs are complex instruments and come with a high > > > risk > > > > of > > > > > > losing money rapidly due to leverage. 70.5% of retail investor > > > accounts > > > > > > lose money when spread betting and/or trading CFDs with this > > > provider. > > > > > You > > > > > > should consider whether you understand how spread bets and CFDs > > work > > > > and > > > > > > whether you can afford to take the high risk of losing your > money. > > > > > > > > > > > > Professional clients: Losses can exceed deposits when spread > > betting > > > > and > > > > > > trading CFDs. Countdowns carry a level of risk to your capital as > > you > > > > > could > > > > > > lose all of your investment. These products may not be suitable > for > > > all > > > > > > clients therefore ensure you understand the risks and seek > > > independent > > > > > > advice. Invest only what you can afford to lose. > > > > > > > > > > > > CMC Markets UK plc (173730) and CMC Spreadbet plc (170627) are > > > > authorised > > > > > > and regulated by the Financial Conduct Authority in the United > > > Kingdom. > > > > > CMC > > > > > > Markets UK plc and CMC Spreadbet plc are registered in England > and > > > > Wales > > > > > > with Company Numbers 02448409 and 02589529 and with their > > registered > > > > > > offices at 133 Houndsditch, London, EC3A 7BX. > > > > > > > > > > > > The content of this e-mail (including any attachments) is > strictly > > > > > > confidential and is for the sole use of the intended > addressee(s). > > If > > > > you > > > > > > are not the intended recipient of this e-mail please notify the > > > sender > > > > > > immediately and delete this e-mail from your system. Any > > disclosure, > > > > > > copying, dissemination or use of its content (including any > > > > attachments) > > > > > is > > > > > > strictly prohibited. CMC Markets UK plc and CMC Spreadbet plc > > reserve > > > > the > > > > > > right to intercept and monitor the content of the e-mail messages > > to > > > > and > > > > > > from its systems. > > > > > > > > > > > > E-mails may be interfered with or may contain viruses or other > > > defects > > > > > for > > > > > > which CMC Markets UK plc and CMC Spreadbet plc accept no > > > > responsibility. > > > > > It > > > > > > is the responsibility of the recipient to carry out a virus check > > on > > > > the > > > > > > e-mail and any attachment(s). > > > > > > > > > > > > This communication is not intended as an offer or solicitation > for > > > the > > > > > > purchase or sale of a financial instrument or as an official > > > > confirmation > > > > > > of any transaction unless specifically presented as such. > > > > > > > > > > > > CMC Markets UK plc and CMC Spreadbet plc are registered in > England > > > and > > > > > > Wales with Company Numbers 02448409 and 02589529 and with their > > > > > registered > > > > > > offices at 133 Houndsditch, London, EC3A 7BX. > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Best regards, > > > Alexei Scherbakov > > > > > > |
Denis,
In general, code changes look good to me. If we decide to keep security API in its current state for a while, I highly recommend to extend its documentation. We don't have descriptive javadocs or articles about security API so far, so I expect that next contributors will face difficulties in untangling security logic. Let's help them a bit. See more details in my JIRA comment: https://issues.apache.org/jira/browse/IGNITE-12759 On Thu, Mar 26, 2020 at 5:54 PM Ivan Rakov <[hidden email]> wrote: > Denis, > > I'll review your PR. If this issue is a subject to be included in 2.8.1 in > emergency mode, I'm ok with the current API changes. > Please think about driving creating IEP on security API overhaul prior to > 2.9. I believe that you are the most suitable Ignite community member to > drive this activity. I'd love to share some ideas as well. > > On Tue, Mar 24, 2020 at 2:04 PM Denis Garus <[hidden email]> wrote: > >> Hi, guys! >> >> >> I agree that we should rework the security API, but it can take a long >> time. >> >> And currently, our users have certain impediments that are blockers for >> their job. >> >> I think we have to fix bugs that IEP-41 [1] contains as soon as possible >> to >> support our users. >> >> From my point of view, IEP-41 is the best place to track bug fixing. >> >> >> >> 1. >> >> https://cwiki.apache.org/confluence/display/IGNITE/IEP-41%3A+Security+Context+of+thin+client+on+remote+nodes >> >> >> вт, 24 мар. 2020 г. в 12:26, Ivan Rakov <[hidden email]>: >> >> > Alexey, >> > >> > That can be another version of our plan. If everyone agrees that >> > SecurityContext and SecuritySubject should be merged, such fix of thin >> > clients' issue will bring us closer to the final solution. >> > Denis, what do you think? >> > >> > On Tue, Mar 24, 2020 at 10:38 AM Alexei Scherbakov < >> > [hidden email]> wrote: >> > >> > > Why can't we start gradually changing security API right now ? >> > > I see no point in delaying with. >> > > All changes will go to next 2.9 release anyway. >> > > >> > > My proposal: >> > > 1. Get rid of security context. Doing this will bring security API to >> > more >> > > or less consistent state. >> > > 2. Remove IEP-41 because it's no longer needed because of change [1] >> > > 3. Propose an IEP to make security API avoid using internals. >> > > >> > > >> > > >> > > пн, 23 мар. 2020 г. в 19:53, Denis Garus <[hidden email]>: >> > > >> > > > Hello, Alexei, Ivan! >> > > > >> > > > >> Seems like security API is indeed a bit over-engineered >> > > > >> > > > Nobody has doubt we should do a reworking of GridSecurityProcessor. >> > > > But this point is outside of scope thin client's problem that we are >> > > > solving. >> > > > I think we can create new IEP that will accumulate all ideas of >> > Ignite's >> > > > security improvements. >> > > > >> > > > >> Presence of the separate #securityContext(UUID) highlights that >> user >> > > > indeed should care >> > > > >> about propagation of thin clients' contexts between the cluster >> > nodes. >> > > > >> > > > I agree with Ivan. I've implemented both variants, >> > > > and I like one with #securityContext(UUID) more. >> > > > >> > > > Could you please take a look at PR [1] for the issue [2]? >> > > > >> > > > 1. https://github.com/apache/ignite/pull/7523 >> > > > 2. https://issues.apache.org/jira/browse/IGNITE-12759 >> > > > >> > > > пн, 23 мар. 2020 г. в 11:45, Ivan Rakov <[hidden email]>: >> > > > >> > > > > Alex, Denis, >> > > > > >> > > > > Seems like security API is indeed a bit over-engineered. >> > > > > >> > > > > Let's get rid of SecurityContext and use SecuritySubject instead. >> > > > > > SecurityContext is just a POJO wrapper over >> > > > > > SecuritySubject's >> > > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. >> > > > > > It's functionality can be easily moved to SecuritySubject. >> > > > > >> > > > > I totally agree. Both subject and context are implemented by >> plugin >> > > > > provider, and I don't see any reason to keep both abstractions, >> > > > especially >> > > > > if we are going to get rid of transferring subject in node >> attributes >> > > > > (argument that subject is more lightweight won't work anymore). >> > > > > >> > > > > Also, there's kind of mess in node authentication logic. There >> are at >> > > > least >> > > > > two components responsible for it: DiscoverySpiNodeAuthenticator >> > (which >> > > > is >> > > > > forcibly set by GridDiscoveryManager, but in fact public) and >> > > > > GridSecurityProcessor (which performs actual node auth logic, but >> > > > private). >> > > > > I also don't understand why we need both >> > > > > #authenticate(AuthenticationContext) and >> > #authenticateNode(ClusterNode, >> > > > > SecurityCredentials) methods while it's possible to set explicit >> > > > > SecuritySubjectType.REMOTE_NODE in AuthenticationContext (this is >> > > > arguable; >> > > > > perhaps there are strong reasons). >> > > > > >> > > > > Finally, areas of responsibility between IgniteSecurity and >> > > > > GridSecurityProcessor are kind of mixed. As far as I understand, >> the >> > > > first >> > > > > is responsible for Ignite-internal management of security logic >> > > (keeping >> > > > > thread-local context, caching security contexts, etc; we don't >> expect >> > > > > IgniteSecurity to be replaced by plugin provider) and the latter >> is >> > > > > responsible for user-custom authentication / authorization logic. >> To >> > be >> > > > > honest, it took plenty of time to figure this out for me. >> > > > > >> > > > > From my point of view, we should make GridSecurityProcessor >> interface >> > > > > public, rename it (it requires plenty of time to find the >> difference >> > > from >> > > > > IgniteSecurity), make its API as simple and non-duplicating as >> > possible >> > > > and >> > > > > clarify its area of responsibility (e.g. should it be responsible >> for >> > > > > propagation of successfully authenticated subject among all nodes >> or >> > > > not?) >> > > > > to make it easy to embed custom security logic in Ignite. >> > > > > >> > > > > Regarding thin clients fix: implementation made by Denis suits >> better >> > > to >> > > > > the very implicit contract that it's better to change API >> contracts >> > of >> > > an >> > > > > internal IgniteSecurity than of internal GridSecurityProcessor >> (which >> > > > > actually mustn't be internal). >> > > > > >> > > > > > My approach doesn't require any IEPs, just minor change in code >> and >> > > to >> > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) >> > > > > > contract. >> > > > > >> > > > > Looks like a misuse of #authenticate method to me. It should >> perform >> > > > > initial authentication based on credentials (this may include >> queries >> > > to >> > > > > external authentication subsystem, e.g. LDAP). User may want to >> don't >> > > > > authenticate thin client on every node (this will increase the >> number >> > > of >> > > > > requests to auth subsystem unless user implicitly implements >> > > propagation >> > > > of >> > > > > thin clients' contexts between nodes and make #authenticate >> > > cluster-wide >> > > > > idempotent: first call should perform actual authentication, next >> > calls >> > > > > should retrieve context of already authenticated client). >> Presence of >> > > the >> > > > > separate #securityContext(UUID) highlights that user indeed should >> > care >> > > > > about propagation of thin clients' contexts between the cluster >> > nodes. >> > > > > >> > > > > -- >> > > > > Ivan >> > > > > >> > > > > On Fri, Mar 20, 2020 at 12:22 PM Veena Mithare < >> > > [hidden email] >> > > > > >> > > > > wrote: >> > > > > >> > > > > > Hi Alexei, Denis, >> > > > > > >> > > > > > One of the main usecases of thin client authentication is to be >> > able >> > > to >> > > > > > audit the changes done using the thin client user. >> > > > > > To enable that : >> > > > > > We really need to resolve this concern as well : >> > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 >> > > > > > >> > > > > > ( Incorrect security subject id is associated with a cache_put >> > event >> > > > > > when the originator of the event is a thin client. ) >> > > > > > >> > > > > > Regards, >> > > > > > Veena >> > > > > > >> > > > > > >> > > > > > -----Original Message----- >> > > > > > From: Alexei Scherbakov <[hidden email]> >> > > > > > Sent: 18 March 2020 08:11 >> > > > > > To: dev <[hidden email]> >> > > > > > Subject: Re: Security Subject of thin client on remote nodes >> > > > > > >> > > > > > Denis Garus, >> > > > > > >> > > > > > Both variants are capable of solving the thin client security >> > context >> > > > > > problem. >> > > > > > >> > > > > > My approach doesn't require any IEPs, just minor change in code >> and >> > > to >> > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) >> > > > > > contract. >> > > > > > We can add appropriate documentation to emphasize this. >> > > > > > The argument "fragile" is not very convincing for me. >> > > > > > >> > > > > > I think we should collect more opinions before proceeding with >> IEP. >> > > > > > >> > > > > > Considering a fact we actually *may not care* about >> compatibility >> > > (I've >> > > > > > already explained why), I'm thinking of another approach. >> > > > > > Let's get rid of SecurityContext and use SecuritySubject >> instead. >> > > > > > SecurityContext is just a POJO wrapper over SecuritySubject's >> > > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. >> > > > > > It's functionality can be easily moved to SecuritySubject. >> > > > > > >> > > > > > What do you think? >> > > > > > >> > > > > > >> > > > > > >> > > > > > пн, 16 мар. 2020 г. в 15:47, Denis Garus <[hidden email]>: >> > > > > > >> > > > > > > Hello, Alexei! >> > > > > > > >> > > > > > > I agree with you if we may not care about compatibility at >> all, >> > > then >> > > > > > > we can solve the problem much more straightforward way. >> > > > > > > >> > > > > > > In your case, the method GridSecurityProcessor#authenticate >> will >> > > have >> > > > > > > an implicit contract: >> > > > > > > [ if actx.subject() != null then >> > > > > > > returns SecurityContext >> > > > > > > else >> > > > > > > do authenticate ] >> > > > > > > >> > > > > > > It looks fragile. >> > > > > > > >> > > > > > > When we extend the GridSecurityProcessor, there isn't this >> > problem: >> > > > > > > we have the explicit contract and can make default >> implementation >> > > > that >> > > > > > > throws an unsupported operation exception to enforcing >> > > compatibility >> > > > > > > check. >> > > > > > > >> > > > > > > In any case, we need to change GridSecurityProcessor >> > > implementation. >> > > > > > > >> > > > > > > But I think your proposal to try to find a security context in >> > the >> > > > > > > node's attributes first is right for backward compatibility >> when >> > > > > > > Ignite users don't use thin clients. >> > > > > > > >> > > > > > > Summary: >> > > > > > > I suggest adding a new method to GridSecurityProcessor >> because it >> > > has >> > > > > > > a clear contract and enforces compatibility check natural way. >> > > > > > > >> > > > > > > вс, 15 мар. 2020 г. в 17:13, Alexei Scherbakov < >> > > > > > > [hidden email] >> > > > > > > >: >> > > > > > > >> > > > > > > > Denis Garus, >> > > > > > > > >> > > > > > > > I've looked at the IEP proposed by you and currently I'm >> > thinking >> > > > > > > > it's >> > > > > > > not >> > > > > > > > immediately required. >> > > > > > > > >> > > > > > > > The problem of missing SecurityContexts of thin clients can >> be >> > > > > > > > solved >> > > > > > > much >> > > > > > > > easily. >> > > > > > > > >> > > > > > > > Below is the stub of a fix, it requires correct >> implementation >> > of >> > > > > > > > method >> > > > > > > > >> > > > > > > >> > > > >> org.apache.ignite.internal.processors.security.IgniteSecurityProcessor >> > > > > > > #authenticatedSubject >> > > > > > > > by GridSecurityProcessor: >> > > > > > > > >> > > > > > > > /** {@inheritDoc} */ >> > > > > > > > @Override public OperationSecurityContext >> withContext(UUID >> > > > > nodeId) >> > > > > > { >> > > > > > > > try { >> > > > > > > > SecurityContext ctx0 = secCtxs.get(nodeId); >> > > > > > > > >> > > > > > > > if (ctx0 == null) { >> > > > > > > > ClusterNode node = >> > > > > > > > Optional.ofNullable(ctx.discovery().node(nodeId)) >> > > > > > > > .orElseGet(() -> >> > > > > > > > ctx.discovery().historicalNode(nodeId)); >> > > > > > > > >> > > > > > > > // This is a cluster node. >> > > > > > > > if (node != null) >> > > > > > > > ctx0 = nodeSecurityContext(marsh, >> > > > > > > > U.resolveClassLoader(ctx.config()), findNode(nodeId)); >> > > > > > > > else { >> > > > > > > > // This is already authenticated thin >> > client. >> > > > > > > > SecuritySubject subj = >> > > > > > > > authenticatedSubject(nodeId); >> > > > > > > > >> > > > > > > > assert subj != null : "Subject is null >> " + >> > > > > > > > nodeId; >> > > > > > > > >> > > > > > > > AuthenticationContext actx = new >> > > > > > > > AuthenticationContext(); >> > > > > > > > actx.subject(subj); >> > > > > > > > >> > > > > > > > ctx0 = secPrc.authenticate(actx); >> > > > > > > > } >> > > > > > > > } >> > > > > > > > >> > > > > > > > secCtxs.putIfAbsent(nodeId, ctx0); >> > > > > > > > >> > > > > > > > return withContext(ctx0); >> > > > > > > > } catch (IgniteCheckedException e) { >> > > > > > > > throw new IgniteException(e); >> > > > > > > > } >> > > > > > > > >> > > > > > > > The idea is to create a thin client SecurityContext on a >> node >> > not >> > > > > > > > having >> > > > > > > a >> > > > > > > > local context using existing SecuritySubject data. >> > > > > > > > >> > > > > > > > Method >> > > > > > > > >> > > > > > > >> > > > >> org.apache.ignite.internal.processors.security.GridSecurityProcessor#a >> > > > > > > uthenticate >> > > > > > > > should check for not null SecuritySubject field and just >> > recreate >> > > > > > > > SecurityContext using passed info (because it's already >> > > > > authenticated). >> > > > > > > > >> > > > > > > > We have all necessary information in SecuritySubject >> returned >> > by >> > > > > > > > >> > > > > > > > >> > > > > > > >> > > > >> org.apache.ignite.internal.processors.security.IgniteSecurityProcessor >> > > > > > > #authenticatedSubject >> > > > > > > > by GridSecurityProcessor method. >> > > > > > > > >> > > > > > > > Because it is internal API, we may not care about >> > compatibility >> > > at >> > > > > > > > all, but nevertheless it is possible to add compatibility >> check >> > > in >> > > > > > > > the method above. If a feature is not supported the >> operations >> > > from >> > > > > > > > thin clients should be forbidden. >> > > > > > > > >> > > > > > > > You proposal has the similar problem: if >> GridSecurityProcessor >> > > does >> > > > > > > > not support retriving context for thin clients, such clients >> > will >> > > > > > > > not be able to proceed with operation. >> > > > > > > > >> > > > > > > > Still, the cleanup of security API is necessary and should >> be >> > > done >> > > > > > > > in 3.0 >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > чт, 12 мар. 2020 г. в 16:48, VeenaMithare < >> > > > [hidden email] >> > > > > >: >> > > > > > > > >> > > > > > > > > HI , >> > > > > > > > > >> > > > > > > > > Created this jira : >> > > > > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 >> > > > > > > > > >> > > > > > > > > regards, >> > > > > > > > > Veena. >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > -- >> > > > > > > > > Sent from: >> > > > http://apache-ignite-developers.2346864.n4.nabble.com/ >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > -- >> > > > > > > > >> > > > > > > > Best regards, >> > > > > > > > Alexei Scherbakov >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > -- >> > > > > > >> > > > > > Best regards, >> > > > > > Alexei Scherbakov >> > > > > > ________________________________ >> > > > > > >> > > > > > >> > > > > > Spread bets and CFDs are complex instruments and come with a >> high >> > > risk >> > > > of >> > > > > > losing money rapidly due to leverage. 70.5% of retail investor >> > > accounts >> > > > > > lose money when spread betting and/or trading CFDs with this >> > > provider. >> > > > > You >> > > > > > should consider whether you understand how spread bets and CFDs >> > work >> > > > and >> > > > > > whether you can afford to take the high risk of losing your >> money. >> > > > > > >> > > > > > Professional clients: Losses can exceed deposits when spread >> > betting >> > > > and >> > > > > > trading CFDs. Countdowns carry a level of risk to your capital >> as >> > you >> > > > > could >> > > > > > lose all of your investment. These products may not be suitable >> for >> > > all >> > > > > > clients therefore ensure you understand the risks and seek >> > > independent >> > > > > > advice. Invest only what you can afford to lose. >> > > > > > >> > > > > > CMC Markets UK plc (173730) and CMC Spreadbet plc (170627) are >> > > > authorised >> > > > > > and regulated by the Financial Conduct Authority in the United >> > > Kingdom. >> > > > > CMC >> > > > > > Markets UK plc and CMC Spreadbet plc are registered in England >> and >> > > > Wales >> > > > > > with Company Numbers 02448409 and 02589529 and with their >> > registered >> > > > > > offices at 133 Houndsditch, London, EC3A 7BX. >> > > > > > >> > > > > > The content of this e-mail (including any attachments) is >> strictly >> > > > > > confidential and is for the sole use of the intended >> addressee(s). >> > If >> > > > you >> > > > > > are not the intended recipient of this e-mail please notify the >> > > sender >> > > > > > immediately and delete this e-mail from your system. Any >> > disclosure, >> > > > > > copying, dissemination or use of its content (including any >> > > > attachments) >> > > > > is >> > > > > > strictly prohibited. CMC Markets UK plc and CMC Spreadbet plc >> > reserve >> > > > the >> > > > > > right to intercept and monitor the content of the e-mail >> messages >> > to >> > > > and >> > > > > > from its systems. >> > > > > > >> > > > > > E-mails may be interfered with or may contain viruses or other >> > > defects >> > > > > for >> > > > > > which CMC Markets UK plc and CMC Spreadbet plc accept no >> > > > responsibility. >> > > > > It >> > > > > > is the responsibility of the recipient to carry out a virus >> check >> > on >> > > > the >> > > > > > e-mail and any attachment(s). >> > > > > > >> > > > > > This communication is not intended as an offer or solicitation >> for >> > > the >> > > > > > purchase or sale of a financial instrument or as an official >> > > > confirmation >> > > > > > of any transaction unless specifically presented as such. >> > > > > > >> > > > > > CMC Markets UK plc and CMC Spreadbet plc are registered in >> England >> > > and >> > > > > > Wales with Company Numbers 02448409 and 02589529 and with their >> > > > > registered >> > > > > > offices at 133 Houndsditch, London, EC3A 7BX. >> > > > > > >> > > > > >> > > > >> > > >> > > >> > > -- >> > > >> > > Best regards, >> > > Alexei Scherbakov >> > > >> > >> > |
Ivan, thank you very much for the review and proposals!
I've made more descriptive JavaDoc; take a look, please. > Please think about driving creating IEP on security API overhaul prior to 2.9. Yes, I'll create IEP on security API soon. I also have some ideas about how we can improve Ignite's security. Thank you! пт, 27 мар. 2020 г. в 11:41, Ivan Rakov <[hidden email]>: > Denis, > > In general, code changes look good to me. If we decide to keep security API > in its current state for a while, I highly recommend to extend its > documentation. We don't have descriptive javadocs or articles about > security API so far, so I expect that next contributors will face > difficulties in untangling security logic. Let's help them a bit. > See more details in my JIRA comment: > https://issues.apache.org/jira/browse/IGNITE-12759 > > On Thu, Mar 26, 2020 at 5:54 PM Ivan Rakov <[hidden email]> wrote: > > > Denis, > > > > I'll review your PR. If this issue is a subject to be included in 2.8.1 > in > > emergency mode, I'm ok with the current API changes. > > Please think about driving creating IEP on security API overhaul prior to > > 2.9. I believe that you are the most suitable Ignite community member to > > drive this activity. I'd love to share some ideas as well. > > > > On Tue, Mar 24, 2020 at 2:04 PM Denis Garus <[hidden email]> wrote: > > > >> Hi, guys! > >> > >> > >> I agree that we should rework the security API, but it can take a long > >> time. > >> > >> And currently, our users have certain impediments that are blockers for > >> their job. > >> > >> I think we have to fix bugs that IEP-41 [1] contains as soon as possible > >> to > >> support our users. > >> > >> From my point of view, IEP-41 is the best place to track bug fixing. > >> > >> > >> > >> 1. > >> > >> > https://cwiki.apache.org/confluence/display/IGNITE/IEP-41%3A+Security+Context+of+thin+client+on+remote+nodes > >> > >> > >> вт, 24 мар. 2020 г. в 12:26, Ivan Rakov <[hidden email]>: > >> > >> > Alexey, > >> > > >> > That can be another version of our plan. If everyone agrees that > >> > SecurityContext and SecuritySubject should be merged, such fix of thin > >> > clients' issue will bring us closer to the final solution. > >> > Denis, what do you think? > >> > > >> > On Tue, Mar 24, 2020 at 10:38 AM Alexei Scherbakov < > >> > [hidden email]> wrote: > >> > > >> > > Why can't we start gradually changing security API right now ? > >> > > I see no point in delaying with. > >> > > All changes will go to next 2.9 release anyway. > >> > > > >> > > My proposal: > >> > > 1. Get rid of security context. Doing this will bring security API > to > >> > more > >> > > or less consistent state. > >> > > 2. Remove IEP-41 because it's no longer needed because of change [1] > >> > > 3. Propose an IEP to make security API avoid using internals. > >> > > > >> > > > >> > > > >> > > пн, 23 мар. 2020 г. в 19:53, Denis Garus <[hidden email]>: > >> > > > >> > > > Hello, Alexei, Ivan! > >> > > > > >> > > > >> Seems like security API is indeed a bit over-engineered > >> > > > > >> > > > Nobody has doubt we should do a reworking of > GridSecurityProcessor. > >> > > > But this point is outside of scope thin client's problem that we > are > >> > > > solving. > >> > > > I think we can create new IEP that will accumulate all ideas of > >> > Ignite's > >> > > > security improvements. > >> > > > > >> > > > >> Presence of the separate #securityContext(UUID) highlights that > >> user > >> > > > indeed should care > >> > > > >> about propagation of thin clients' contexts between the cluster > >> > nodes. > >> > > > > >> > > > I agree with Ivan. I've implemented both variants, > >> > > > and I like one with #securityContext(UUID) more. > >> > > > > >> > > > Could you please take a look at PR [1] for the issue [2]? > >> > > > > >> > > > 1. https://github.com/apache/ignite/pull/7523 > >> > > > 2. https://issues.apache.org/jira/browse/IGNITE-12759 > >> > > > > >> > > > пн, 23 мар. 2020 г. в 11:45, Ivan Rakov <[hidden email]>: > >> > > > > >> > > > > Alex, Denis, > >> > > > > > >> > > > > Seems like security API is indeed a bit over-engineered. > >> > > > > > >> > > > > Let's get rid of SecurityContext and use SecuritySubject > instead. > >> > > > > > SecurityContext is just a POJO wrapper over > >> > > > > > SecuritySubject's > >> > > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > >> > > > > > It's functionality can be easily moved to SecuritySubject. > >> > > > > > >> > > > > I totally agree. Both subject and context are implemented by > >> plugin > >> > > > > provider, and I don't see any reason to keep both abstractions, > >> > > > especially > >> > > > > if we are going to get rid of transferring subject in node > >> attributes > >> > > > > (argument that subject is more lightweight won't work anymore). > >> > > > > > >> > > > > Also, there's kind of mess in node authentication logic. There > >> are at > >> > > > least > >> > > > > two components responsible for it: DiscoverySpiNodeAuthenticator > >> > (which > >> > > > is > >> > > > > forcibly set by GridDiscoveryManager, but in fact public) and > >> > > > > GridSecurityProcessor (which performs actual node auth logic, > but > >> > > > private). > >> > > > > I also don't understand why we need both > >> > > > > #authenticate(AuthenticationContext) and > >> > #authenticateNode(ClusterNode, > >> > > > > SecurityCredentials) methods while it's possible to set explicit > >> > > > > SecuritySubjectType.REMOTE_NODE in AuthenticationContext (this > is > >> > > > arguable; > >> > > > > perhaps there are strong reasons). > >> > > > > > >> > > > > Finally, areas of responsibility between IgniteSecurity and > >> > > > > GridSecurityProcessor are kind of mixed. As far as I understand, > >> the > >> > > > first > >> > > > > is responsible for Ignite-internal management of security logic > >> > > (keeping > >> > > > > thread-local context, caching security contexts, etc; we don't > >> expect > >> > > > > IgniteSecurity to be replaced by plugin provider) and the latter > >> is > >> > > > > responsible for user-custom authentication / authorization > logic. > >> To > >> > be > >> > > > > honest, it took plenty of time to figure this out for me. > >> > > > > > >> > > > > From my point of view, we should make GridSecurityProcessor > >> interface > >> > > > > public, rename it (it requires plenty of time to find the > >> difference > >> > > from > >> > > > > IgniteSecurity), make its API as simple and non-duplicating as > >> > possible > >> > > > and > >> > > > > clarify its area of responsibility (e.g. should it be > responsible > >> for > >> > > > > propagation of successfully authenticated subject among all > nodes > >> or > >> > > > not?) > >> > > > > to make it easy to embed custom security logic in Ignite. > >> > > > > > >> > > > > Regarding thin clients fix: implementation made by Denis suits > >> better > >> > > to > >> > > > > the very implicit contract that it's better to change API > >> contracts > >> > of > >> > > an > >> > > > > internal IgniteSecurity than of internal GridSecurityProcessor > >> (which > >> > > > > actually mustn't be internal). > >> > > > > > >> > > > > > My approach doesn't require any IEPs, just minor change in > code > >> and > >> > > to > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > >> > > > > > contract. > >> > > > > > >> > > > > Looks like a misuse of #authenticate method to me. It should > >> perform > >> > > > > initial authentication based on credentials (this may include > >> queries > >> > > to > >> > > > > external authentication subsystem, e.g. LDAP). User may want to > >> don't > >> > > > > authenticate thin client on every node (this will increase the > >> number > >> > > of > >> > > > > requests to auth subsystem unless user implicitly implements > >> > > propagation > >> > > > of > >> > > > > thin clients' contexts between nodes and make #authenticate > >> > > cluster-wide > >> > > > > idempotent: first call should perform actual authentication, > next > >> > calls > >> > > > > should retrieve context of already authenticated client). > >> Presence of > >> > > the > >> > > > > separate #securityContext(UUID) highlights that user indeed > should > >> > care > >> > > > > about propagation of thin clients' contexts between the cluster > >> > nodes. > >> > > > > > >> > > > > -- > >> > > > > Ivan > >> > > > > > >> > > > > On Fri, Mar 20, 2020 at 12:22 PM Veena Mithare < > >> > > [hidden email] > >> > > > > > >> > > > > wrote: > >> > > > > > >> > > > > > Hi Alexei, Denis, > >> > > > > > > >> > > > > > One of the main usecases of thin client authentication is to > be > >> > able > >> > > to > >> > > > > > audit the changes done using the thin client user. > >> > > > > > To enable that : > >> > > > > > We really need to resolve this concern as well : > >> > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > >> > > > > > > >> > > > > > ( Incorrect security subject id is associated with a > cache_put > >> > event > >> > > > > > when the originator of the event is a thin client. ) > >> > > > > > > >> > > > > > Regards, > >> > > > > > Veena > >> > > > > > > >> > > > > > > >> > > > > > -----Original Message----- > >> > > > > > From: Alexei Scherbakov <[hidden email]> > >> > > > > > Sent: 18 March 2020 08:11 > >> > > > > > To: dev <[hidden email]> > >> > > > > > Subject: Re: Security Subject of thin client on remote nodes > >> > > > > > > >> > > > > > Denis Garus, > >> > > > > > > >> > > > > > Both variants are capable of solving the thin client security > >> > context > >> > > > > > problem. > >> > > > > > > >> > > > > > My approach doesn't require any IEPs, just minor change in > code > >> and > >> > > to > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > >> > > > > > contract. > >> > > > > > We can add appropriate documentation to emphasize this. > >> > > > > > The argument "fragile" is not very convincing for me. > >> > > > > > > >> > > > > > I think we should collect more opinions before proceeding with > >> IEP. > >> > > > > > > >> > > > > > Considering a fact we actually *may not care* about > >> compatibility > >> > > (I've > >> > > > > > already explained why), I'm thinking of another approach. > >> > > > > > Let's get rid of SecurityContext and use SecuritySubject > >> instead. > >> > > > > > SecurityContext is just a POJO wrapper over SecuritySubject's > >> > > > > > org.apache.ignite.plugin.security.SecuritySubject#permissions. > >> > > > > > It's functionality can be easily moved to SecuritySubject. > >> > > > > > > >> > > > > > What do you think? > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > пн, 16 мар. 2020 г. в 15:47, Denis Garus <[hidden email] > >: > >> > > > > > > >> > > > > > > Hello, Alexei! > >> > > > > > > > >> > > > > > > I agree with you if we may not care about compatibility at > >> all, > >> > > then > >> > > > > > > we can solve the problem much more straightforward way. > >> > > > > > > > >> > > > > > > In your case, the method GridSecurityProcessor#authenticate > >> will > >> > > have > >> > > > > > > an implicit contract: > >> > > > > > > [ if actx.subject() != null then > >> > > > > > > returns SecurityContext > >> > > > > > > else > >> > > > > > > do authenticate ] > >> > > > > > > > >> > > > > > > It looks fragile. > >> > > > > > > > >> > > > > > > When we extend the GridSecurityProcessor, there isn't this > >> > problem: > >> > > > > > > we have the explicit contract and can make default > >> implementation > >> > > > that > >> > > > > > > throws an unsupported operation exception to enforcing > >> > > compatibility > >> > > > > > > check. > >> > > > > > > > >> > > > > > > In any case, we need to change GridSecurityProcessor > >> > > implementation. > >> > > > > > > > >> > > > > > > But I think your proposal to try to find a security context > in > >> > the > >> > > > > > > node's attributes first is right for backward compatibility > >> when > >> > > > > > > Ignite users don't use thin clients. > >> > > > > > > > >> > > > > > > Summary: > >> > > > > > > I suggest adding a new method to GridSecurityProcessor > >> because it > >> > > has > >> > > > > > > a clear contract and enforces compatibility check natural > way. > >> > > > > > > > >> > > > > > > вс, 15 мар. 2020 г. в 17:13, Alexei Scherbakov < > >> > > > > > > [hidden email] > >> > > > > > > >: > >> > > > > > > > >> > > > > > > > Denis Garus, > >> > > > > > > > > >> > > > > > > > I've looked at the IEP proposed by you and currently I'm > >> > thinking > >> > > > > > > > it's > >> > > > > > > not > >> > > > > > > > immediately required. > >> > > > > > > > > >> > > > > > > > The problem of missing SecurityContexts of thin clients > can > >> be > >> > > > > > > > solved > >> > > > > > > much > >> > > > > > > > easily. > >> > > > > > > > > >> > > > > > > > Below is the stub of a fix, it requires correct > >> implementation > >> > of > >> > > > > > > > method > >> > > > > > > > > >> > > > > > > > >> > > > > >> org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > >> > > > > > > #authenticatedSubject > >> > > > > > > > by GridSecurityProcessor: > >> > > > > > > > > >> > > > > > > > /** {@inheritDoc} */ > >> > > > > > > > @Override public OperationSecurityContext > >> withContext(UUID > >> > > > > nodeId) > >> > > > > > { > >> > > > > > > > try { > >> > > > > > > > SecurityContext ctx0 = secCtxs.get(nodeId); > >> > > > > > > > > >> > > > > > > > if (ctx0 == null) { > >> > > > > > > > ClusterNode node = > >> > > > > > > > Optional.ofNullable(ctx.discovery().node(nodeId)) > >> > > > > > > > .orElseGet(() -> > >> > > > > > > > ctx.discovery().historicalNode(nodeId)); > >> > > > > > > > > >> > > > > > > > // This is a cluster node. > >> > > > > > > > if (node != null) > >> > > > > > > > ctx0 = nodeSecurityContext(marsh, > >> > > > > > > > U.resolveClassLoader(ctx.config()), findNode(nodeId)); > >> > > > > > > > else { > >> > > > > > > > // This is already authenticated thin > >> > client. > >> > > > > > > > SecuritySubject subj = > >> > > > > > > > authenticatedSubject(nodeId); > >> > > > > > > > > >> > > > > > > > assert subj != null : "Subject is null > >> " + > >> > > > > > > > nodeId; > >> > > > > > > > > >> > > > > > > > AuthenticationContext actx = new > >> > > > > > > > AuthenticationContext(); > >> > > > > > > > actx.subject(subj); > >> > > > > > > > > >> > > > > > > > ctx0 = secPrc.authenticate(actx); > >> > > > > > > > } > >> > > > > > > > } > >> > > > > > > > > >> > > > > > > > secCtxs.putIfAbsent(nodeId, ctx0); > >> > > > > > > > > >> > > > > > > > return withContext(ctx0); > >> > > > > > > > } catch (IgniteCheckedException e) { > >> > > > > > > > throw new IgniteException(e); > >> > > > > > > > } > >> > > > > > > > > >> > > > > > > > The idea is to create a thin client SecurityContext on a > >> node > >> > not > >> > > > > > > > having > >> > > > > > > a > >> > > > > > > > local context using existing SecuritySubject data. > >> > > > > > > > > >> > > > > > > > Method > >> > > > > > > > > >> > > > > > > > >> > > > > >> org.apache.ignite.internal.processors.security.GridSecurityProcessor#a > >> > > > > > > uthenticate > >> > > > > > > > should check for not null SecuritySubject field and just > >> > recreate > >> > > > > > > > SecurityContext using passed info (because it's already > >> > > > > authenticated). > >> > > > > > > > > >> > > > > > > > We have all necessary information in SecuritySubject > >> returned > >> > by > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > >> org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > >> > > > > > > #authenticatedSubject > >> > > > > > > > by GridSecurityProcessor method. > >> > > > > > > > > >> > > > > > > > Because it is internal API, we may not care about > >> > compatibility > >> > > at > >> > > > > > > > all, but nevertheless it is possible to add compatibility > >> check > >> > > in > >> > > > > > > > the method above. If a feature is not supported the > >> operations > >> > > from > >> > > > > > > > thin clients should be forbidden. > >> > > > > > > > > >> > > > > > > > You proposal has the similar problem: if > >> GridSecurityProcessor > >> > > does > >> > > > > > > > not support retriving context for thin clients, such > clients > >> > will > >> > > > > > > > not be able to proceed with operation. > >> > > > > > > > > >> > > > > > > > Still, the cleanup of security API is necessary and should > >> be > >> > > done > >> > > > > > > > in 3.0 > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > чт, 12 мар. 2020 г. в 16:48, VeenaMithare < > >> > > > [hidden email] > >> > > > > >: > >> > > > > > > > > >> > > > > > > > > HI , > >> > > > > > > > > > >> > > > > > > > > Created this jira : > >> > > > > > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > >> > > > > > > > > > >> > > > > > > > > regards, > >> > > > > > > > > Veena. > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > -- > >> > > > > > > > > Sent from: > >> > > > http://apache-ignite-developers.2346864.n4.nabble.com/ > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > -- > >> > > > > > > > > >> > > > > > > > Best regards, > >> > > > > > > > Alexei Scherbakov > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > >> > > > > > -- > >> > > > > > > >> > > > > > Best regards, > >> > > > > > Alexei Scherbakov > >> > > > > > ________________________________ > >> > > > > > > >> > > > > > > >> > > > > > Spread bets and CFDs are complex instruments and come with a > >> high > >> > > risk > >> > > > of > >> > > > > > losing money rapidly due to leverage. 70.5% of retail investor > >> > > accounts > >> > > > > > lose money when spread betting and/or trading CFDs with this > >> > > provider. > >> > > > > You > >> > > > > > should consider whether you understand how spread bets and > CFDs > >> > work > >> > > > and > >> > > > > > whether you can afford to take the high risk of losing your > >> money. > >> > > > > > > >> > > > > > Professional clients: Losses can exceed deposits when spread > >> > betting > >> > > > and > >> > > > > > trading CFDs. Countdowns carry a level of risk to your capital > >> as > >> > you > >> > > > > could > >> > > > > > lose all of your investment. These products may not be > suitable > >> for > >> > > all > >> > > > > > clients therefore ensure you understand the risks and seek > >> > > independent > >> > > > > > advice. Invest only what you can afford to lose. > >> > > > > > > >> > > > > > CMC Markets UK plc (173730) and CMC Spreadbet plc (170627) are > >> > > > authorised > >> > > > > > and regulated by the Financial Conduct Authority in the United > >> > > Kingdom. > >> > > > > CMC > >> > > > > > Markets UK plc and CMC Spreadbet plc are registered in England > >> and > >> > > > Wales > >> > > > > > with Company Numbers 02448409 and 02589529 and with their > >> > registered > >> > > > > > offices at 133 Houndsditch, London, EC3A 7BX. > >> > > > > > > >> > > > > > The content of this e-mail (including any attachments) is > >> strictly > >> > > > > > confidential and is for the sole use of the intended > >> addressee(s). > >> > If > >> > > > you > >> > > > > > are not the intended recipient of this e-mail please notify > the > >> > > sender > >> > > > > > immediately and delete this e-mail from your system. Any > >> > disclosure, > >> > > > > > copying, dissemination or use of its content (including any > >> > > > attachments) > >> > > > > is > >> > > > > > strictly prohibited. CMC Markets UK plc and CMC Spreadbet plc > >> > reserve > >> > > > the > >> > > > > > right to intercept and monitor the content of the e-mail > >> messages > >> > to > >> > > > and > >> > > > > > from its systems. > >> > > > > > > >> > > > > > E-mails may be interfered with or may contain viruses or other > >> > > defects > >> > > > > for > >> > > > > > which CMC Markets UK plc and CMC Spreadbet plc accept no > >> > > > responsibility. > >> > > > > It > >> > > > > > is the responsibility of the recipient to carry out a virus > >> check > >> > on > >> > > > the > >> > > > > > e-mail and any attachment(s). > >> > > > > > > >> > > > > > This communication is not intended as an offer or solicitation > >> for > >> > > the > >> > > > > > purchase or sale of a financial instrument or as an official > >> > > > confirmation > >> > > > > > of any transaction unless specifically presented as such. > >> > > > > > > >> > > > > > CMC Markets UK plc and CMC Spreadbet plc are registered in > >> England > >> > > and > >> > > > > > Wales with Company Numbers 02448409 and 02589529 and with > their > >> > > > > registered > >> > > > > > offices at 133 Houndsditch, London, EC3A 7BX. > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > >> > > -- > >> > > > >> > > Best regards, > >> > > Alexei Scherbakov > >> > > > >> > > >> > > > |
Free forum by Nabble | Edit this page |