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 < alexey.scherbak...@gmail.com> 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 <garus....@gmail.com>: > > > 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 <ivan.glu...@gmail.com>: > > > > > 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 < > v.mith...@cmcmarkets.com > > > > > > 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 <alexey.scherbak...@gmail.com> > > > > Sent: 18 March 2020 08:11 > > > > To: dev <dev@ignite.apache.org> > > > > 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 <garus....@gmail.com>: > > > > > > > > > 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 < > > > > > alexey.scherbak...@gmail.com > > > > > >: > > > > > > > > > > > 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 < > > v.mith...@cmcmarkets.com > > > >: > > > > > > > > > > > > > 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 >
