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
