FWD » Core Development » Base Language

Implement support for the following:

SECURITY-POLICY resource

• SECURITY-POLICY system handle
• attributes
  • SYMMETRIC-ENCRYPTION-ALGORITHM
  • PBE-KEY-ROUNDS
  • PBE-HASH-ALGORITHM
• methods
  • SET-CLIENT
  • REGISTER-DOMAIN
  • LOCK-REGISTRATION

AUDIT-POLICY resource

• AUDIT-POLICY system handle
• methods
  • ENCRYPT-AUDIT-MAC-KEY

AUDIT-CONTROL resource

• AUDIT-CONTROL system handle
• methods
  • LOG-AUDIT-EVENT

Updates to CLIENT-PRINCIPAL

• Integration/support of the full SECURITY-POLICY:SYMMETRIC-ENCRYPTION-ALGORITHM implementation.
• GET-DB-CLIENT() function
• SET-DB-CLIENT() function
• attributes
  • DOMAIN-DESCRIPTION
  • CLIENT-TTY
  • AUDIT-EVENT-CONTEXT
  • LOGIN-HOST
  • ROLES
• methods
  • VALIDATE-SEAL
  • LOGOUT
  • LOCK-REGISTRATION
  • REGISTER-DOMAIN

Other Security Features

• GENERATE-PBE-KEY() runtime
• GENERATE-PBE-SALT runtime

LE (GES): The AUDIT-POLICY and AUDIT-CONTROL work was moved to #6422. Some remaining authentication features were moved to #6423.

Greg Shah wrote:

Constantin: Do you have any notes about the aspects of CLIENT-PRINCIPAL or other features above which need to be implemented?

• encryptDecryptWorker:3350 related to SECURITY-POLICY:SYMMETRIC-ENCRYPTION-KEY
• encryptDecryptWorker:3466, algorithm:2838 and generateRandomKey:1718 related to encryption algorithm via SECURITY-POLICY:SYMMETRIC-ENCRYPTION-ALGORITHM.
• validateClient:3521, used by SET-DB-CLIENT - the authentication for an unsealed CLIENT-PRINCIPAL is not performed in FWD... we need tests to determine how this is performed.
• getDbClient and setDbClient. You can review how ConnectionManager.clients is used.

• exportPrincipal - this must properly serialize the instance. We need to determine how 4GL serializes the CLIENT-PRINCIPAL instance - my assumption is they just serialize the resource attributes, but we need tests to determine how this is done. I think this is the most time-consuming part, testing-wise.
• seal - other (not yet added) attributes are set when this is called. But i.e. SEAL-TIMESTAMP (and others) are not included in #3810-1. And I think this affects the EXPORT-PRINCIPAL and IMPORT-PRINCIPAL support - we can't have full compatibility for these if we don't support all attributes.

Igor, as a side note: when implementing the runtime, if it makes sense to add support for an attribute/method not included in this task (but part of SECURITY-POLICY or CLIENT-PRINCIPAL resources), so that the runtime can work more naturally, go ahead and add conversion and runtime support for it. If you have doubts, please ask.

Igor, do you have an ETA for the conversion side of this task (and stubbed runtime)? At least these attributes are required soon:

pbe-hash-algorithm
pbe-key-rounds
symmetric-encryption-algorithm

Constantin.
Sorry, I forgot to set watch for this task and overlooked your question.
I will try to implement conversion for the attributes in question by tomorrow.

Do we still add multiple signatures for methods with character argument(s) or there is another solution? For example should I define 2^2 + 2^3 + 2^4 = 28 methods for SECURITY-POLICY:REGISTER-DOMAIN?

Maybe it is better to make character (or even Text) implement CharSequence interface (there are only 3 methods to be added)?
What do you think?

No, don't add 28 methods - just make sure the conversion rules always wrap a string literal into a new character(literal).

Constantin Asofiei wrote:

No, don't add 28 methods - just make sure the conversion rules always wrap a string literal into a new character(literal).

See base_structure.rules - I think is enough to add a wrap=true annotation when you are on the AST for an argument in the SECURITY-POLICY:REGISTER-DOMAIN method call.

Thank you, Constantin! It works.

Added conversion support for

SECURITY-POLICY system handle               kw_secur_p
    attributes
        SYMMETRIC-ENCRYPTION-ALGORITHM      kw_sym_en_a
        SYMMETRIC-ENCRYPTION-KEY            kw_sym_en_k
        SYMMETRIC-SUPPORT                   kw_sym_supp
        SYMMETRIC-ENCRYPTION-IV             kw_sym_en_i
        ENCRYPTION-SALT                     kw_enc_salt       
        PBE-KEY-ROUNDS                      kw_pbe_keyr
        PBE-HASH-ALGORITHM                  kw_pbe_h_al
    methods
        GET-CLIENT                          kw_get_clnt
        SET-CLIENT                          kw_set_clnt
        REGISTER-DOMAIN                     kw_reg_dmn
        LOCK-REGISTRATION                   kw_lock_reg

Branch 3810a rev. 11309

Code Review Task Branch 3810a Revision 11309

The first pass is good. Some items:

1. In handle.java, you changed references from DataSourceInterface to DataSource. I think that is incorrect.

2. Since you last were working in this code, we have added a "gap marking" phase of the front end. The idea is that we run TRPL (see rules/gaps/) to mark each node with a conversion support level and a runtime support level. To understand the support levels, please see Gap Analysis. When adding new conversion or runtime support, it is important to update the rules in rules/gaps/ to match the new status. The attributes and methods will be found in rules/gaps/expressions.rules. We don't mark system handles yet.

3. I'd like you to add the conversion for AUDIT-*, CLIENT-PRINCIPAL and other features in #3801-2, before you work on the runtime. The idea is to get these into the trunk and work on the runtime in a 3810b branch.

4. Please add a file header and javadoc to CommonSecurityPolicy and SecurityPolicyManager. In these files the spacing of indents is not matching the standard (3 spaces, no tabs).

5. Please add history entries to all files.

1. Fixed handle
2. Added/updated gaps data
3. -
4. Added file header and javadoc to CommonSecurityPolicy and SecurityPolicyManager. Fixed formatting
5. Added history entries.

Committed to branch 3810a rev. 11311.

Added conversion support for

AUDIT-POLICY system handle                  kw_aud_pol
    methods
        ENCRYPT-AUDIT-MAC-KEY               kw_enc_amk
        REFRESH-AUDIT-POLICY                kw_refr_a_p

Branch 3810a rev. 11312

Added conversion support for

AUDIT-CONTROL system handle                 kw_aud_ctrl
    attributes
        APPL-CONTEXT-ID                     kw_appl_cid
        EVENT-GROUP-ID                      kw_evt_grid
    methods
        BEGIN-EVENT-GROUP                   kw_beg_ev_g
        END-EVENT-GROUP                     kw_end_ev_g
        SET-APPL-CONTEXT                    kw_set_actx
        CLEAR-APPL-CONTEXT                  kw_clr_ap_c
        LOG-AUDIT-EVENT                     kw_log_a_ev

Branch 3810a rev. 11313

Added testcases/uast/audit/audit-control.p, testcases/uast/audit/audit-policy.p

Added conversion support for:

CLIENT-PRINCIPAL                            kw_clnt_prl
    attributes
        DOMAIN-DESCRIPTION                  kw_domain_d         *
        CLIENT-TTY                          kw_clnt_tty
        AUDIT-EVENT-CONTEXT                 kw_aud_ev_c
        LOGIN-HOST                          kw_log_host
        ROLES                               kw_roles
    methods
        VALIDATE-SEAL                       kw_val_seal
        LOGOUT                              kw_logout

The LOCK-REGISTRATION and REGISTER-DOMAIN mentioned in the task description are methods of SECURITY-POLICY, not CLIENT-PRINCIPAL

Branch 3810a rev. 11314

Added testcases/uast/security/client-principal.p

The conversion support for all attributes/methods in the scope of this task is finished.
I understand that it was supposed to be merged into the trunk and a new branch should be created for the rest of the task.
What should I do for this?

Code Review Task Branch 3810a Revision 11314

Good update.

1. The gaps marking for the new items should have runtime support level set to rt_lvl_stub since the stubs are fully there. Reporting these as rt_lvl_none means that the converted code won't compile because there are no stubs.

2. AuditControlManager and AuditPolicyManager need class level javadoc.

3. I've checked in some minor header formatting changes (see rev 11315). You should only have 1 history number for all your changes in one file in a branch.

Once the code review items are resolved, I think it makes sense to merge this branch into 3751a (and then dead-archive 3810a). You can do the runtime implementation in a new branch 3810b.

Constantin: Are you OK with this plan?

Greg Shah wrote:

Once the code review items are resolved, I think it makes sense to merge this branch into 3751a (and then dead-archive 3810a). You can do the runtime implementation in a new branch 3810b.

Constantin: Are you OK with this plan?

Not really. I'd like to have this in trunk, after conversion regression testing. Is easier for me to maintain 3751a this way.

Also, considering #4073 issue: as HandleCommon was extended with new interfaces, the runtime will fail without that patch. So I'd like to have Ovidiu's patch from #4073 in #3810a, and in trunk.

Constantin Asofiei wrote:

Also, considering #4073 issue: as HandleCommon was extended with new interfaces, the runtime will fail without that patch. So I'd like to have Ovidiu's patch from #4073 in #3810a, and in trunk.

And related to runtime: just do a run of ChUI runtime regression testing, and if no obvious abends, then we can consider it passed.

OK.

Ovidiu: Please apply your patch from #4073 to 3810a.

Igor: After your code review resolutions (and Ovidiu's patch is applied), please run regression ChUI regression testing.

Greg Shah wrote:

Ovidiu: Please apply your patch from #4073 to 3810a.

Committed as revision 11316 in branch 3810a.

means that the converted code won't compile because there are no stubs.
Fixed

2. AuditControlManager and AuditPolicyManager need class level javadoc.

Class javadoc added t@AuditControlManager@ and AuditPolicyManager and SecurityPoliceManager

Committed to branch 3819a rev. 11317.

Everything looks good. Please run ChUI regression testing (both conversion and runtime).

Regression testing started.

The results of regression testing (2 runs)"
1. All gso_ctrlc_tests passed in both runs.
2. All gso_ctrlc_3way_tests failed in both runs.
3. In main tests the following tests failed in both runs:
gso_29, gso_185, gso_190, gso_255, gso_265, gso_273, gso_278, gso_319, gso385_session, gso385_session2, logout_from_main_menu (#261)
tc_item_master_005, tc_item_master_028, tc_item_master_082, tc_job_002, tc_item_master_006, tc_item_master_104
4. No exceptions found in server and client logs. (in both runs).

Try running some of these tests individually; there is a run_single.sh and majic_single.xml in majic_baseline/ - you need to edit the majic_single.xml to specify a test, and just do run_single.sh to execute it.

I've ran these and I think all are false-negatives - as they pass, except some with DB or other state dependencies.

Igor, if I don't hear from you in the next 4-6 hours, I'll merge 3810a to trunk later today. Post a message here if you plan to merge it.

Hi Constantin. I will merge 3810a now.

Don'r forget to rebase.

Rebased to the trunk rev. 11307. The new revision is 11318

The branch 3810a was merged into the trunk rev. 11308 and archived.

Created task branch 3810b from trunk rev.11308

Added runtime support for SECURITY-POLICY attributes.
Committed to the branch 3810b rev. 11310.

Synopsys:
If the new value of the SYMMETRIC-ENCRYPTION-ALGORITHM, PBE-KEY-ROUNDS or PBE-HASH-ALGORITHM is UNKNOWN or invalid ( is non-positive for the PBE-KEY-ROUNDS or is not in the set of the supported algorithms for SYMMETRIC-ENCRYPTION-ALGORITHM and PBE-HASH-ALGORITHM), ABL displays two messages:

Invalid $(attribute name) value ($(code))
Unable to set attribute $(attribute name) in widget  of type PSEUDO-Widget (3131)

The code is 12221, 12222 and 12223 for PBE-HASH-ALGORITHM, PBE-KEY-ROUNDS and SYMMETRIC-ENCRYPTION-ALGORITHM respectively.

The validation of the names of the algorithms are case-insensitive and they are saved as-is.

I have not found any limitations for the values of the raw attributes (SYMMETRIC-ENCRYPTION-IV, ENCRYPTION-SALT, and SYMMETRIC-ENCRYPTION-KEY) so far which is a little bit strange, at least for the key and iv. Maybe the validation is performed when they are used.

Do I understand correctly that now it is time to provide "real" implementations of the ENCRYPT and DECRYPT functions (changing the corresponding code from the SecurityOps)?
Thank you.

Another question.
Do we already have an implementation (stub) for the "ABL session domain registry" or it should be implemented in the scope of this task? I understand that it can be in-memory registry as we do not have even conversion support for the SECURITY-POLICY:LOAD-DOMAINS().
Please advise.
Thank you.

Igor Skornyakov wrote:

Do I understand correctly that now it is time to provide "real" implementations of the ENCRYPT and DECRYPT functions (changing the corresponding code from the SecurityOps)?
Thank you.

Yes.

Igor Skornyakov wrote:

Another question.
Do we already have an implementation (stub) for the "ABL session domain registry" or it should be implemented in the scope of this task? I understand that it can be in-memory registry as we do not have even conversion support for the SECURITY-POLICY:LOAD-DOMAINS().
Please advise.
Thank you.

We have noting so far, so we will need it implemented here. As far as in-memory that is OK, but try to avoid approaches that will make it harder for us to finish the loading features (or other unimplemented features) in the future.

Greg Shah wrote:

We have noting so far, so we will need it implemented here. As far as in-memory that is OK, but try to avoid approaches that will make it harder for us to finish the loading features (or other unimplemented features) in the future.

Thank you. I plan to encapsulate the functionality into a "service" implementing some interface so that the implementation can be "pluggable".

Added full runtime support for ENCRYPT/DECRYPT based on SECURITY-POLICY.

Branch 3810b rev.11311.

After thorough testing I've found the following:
1. In an OFB and CFB modes, and WITH RC4 ABL doesn't use padding, as expected
2. Contrary to the common practice ABL doesn't prepend IV to the encrypted text. So it is difficult to understand what they do if the length of the provided IV doesn't match the selected algorithm (we just truncate the data or append zero bytes).
3. There is a strange behavior regarding IV. If the SECURITY-POLICY:SYMMETRIC-ENCRYPTION-IV attribute value was changed after calling ENCRYPT(data) and before calling DECRYPT(data) with the new RAW created from the same CHARACTER value, the result of the decryption is not the same as the original text.
I'm not sure if we have to implement this.

Some 4GL testcases are being written to explore and demonstrate the features in this task. The testcases can be found in the Testcases Project.

The developers that wrote those tests will be posting some details about the tests including answers to questions listed in this task.

Igor: if you have any other questions that need to be answered and have not been previously listed, please post them here.

There is a strange behavior regarding IV. If the SECURITY-POLICY:SYMMETRIC-ENCRYPTION-IV attribute value was changed after calling ENCRYPT and before calling DECRYPT with the new RAW created from the same CHARACTER value, the result of the decryption is not the same as the original text.
I'm not sure if we have to implement this.

This sounds as if the current encryption/decryption just uses the current value of SECURITY-POLICY:SYMMETRIC-ENCRYPTION-IV instead of remembering the one associated with the data. The documentation also suggests this is the case. I think we probably need to implement it this same way because there may be code that depends on this approach. For example, what if some code encrypted/decrypted multiple values and changed the SECURITY-POLICY:SYMMETRIC-ENCRYPTION-IV for each one? If the code is written this way, we must match the original 4GL behavior. It seems to me that the 4GL implementation assumes the caller is managing this value themselves and it doesn't do much other than just store and use the value with each call.

Can you be more specific with your concern?

This sounds as if the current encryption/decryption just uses the current value of SECURITY-POLICY:SYMMETRIC-ENCRYPTION-IV instead of remembering the one associated with the data. The documentation also suggests this is the case. I think we probably need to implement it this same way because there may be code that depends on this approach. For example, what if some code encrypted/decrypted multiple values and changed the SECURITY-POLICY:SYMMETRIC-ENCRYPTION-IV for each one? If the code is written this way, we must match the original 4GL behavior. It seems to me that the 4GL implementation assumes the caller is managing this value themselves and it doesn't do much other than just store and use the value with each call.

Can you be more specific with your concern?

Well, my concern is that in my test (see testcases/uast/security/crypto.p) it looks like we have to use exactly the same instance of RAW IV value. This looks strange. I will continue testing.

Igor, you have a flow in your test:

       IF change-iv THEN DO:
...
           iv-hex = HEX-ENCODE(raw-iv).
           MESSAGE iv-hex.

I think the IV is padded with random bytes - is there a fixed length for the IV, depending on the algorithm?

OK, the random bytes are originated by this:

PUT-STRING(raw-iv1, 1, iv-len) = "iv".

You force raw-iv1 to read iv-len bytes but you have only the "iv" text - and PUT-STRING will read past the "iv" value, instead of crashing down.

To make this predictable, you need to use PUT-STRING(raw-iv1, 1) = "iv". - this will read no more than the number of bytes in the rvalue.

The same for PUT-STRING(raw-key, 1, key-len) = "key".. Don't specify the number of bytes being written UNLESS the rvalue has at least the number of bytes you expect to write.

You force raw-iv1 to read iv-len bytes but you have only the "iv" text - and PUT-STRING will read past the "iv" value, instead of crashing down.

To make this predictable, you need to use PUT-STRING(raw-iv1, 1) = "iv". - this will read no more than the number of bytes in the rvalue.

The same for PUT-STRING(raw-key, 1, key-len) = "key".. Don't specify the number of bytes being written UNLESS the rvalue has at least the number of bytes you expect to write.

Oh, I see. Thanks a lot! I will fix my test. Yes, each block cipher requires the specific length of the IV. This is encoded in SecurityPolicyManager.CipherParams.cipher() method.

Added quotes from the mail conversation with Marian:

On 5/17/19 8:31 AM, Marian Edu wrote:

Hi Igor,
you're right about the initialization vector, Progress does not mention any requirements on it's size and normally this should match the cipher's block size for block ciphers but I've couldn't find that to be an express requirement. What we've found is that pass a certain length the rest of the vector data is ignored, for default algorithm this is set to 16 byte so if the first bytes of the IV matches the result of encode/decode is the same. We're trying to see how the IV is padded if less than that is provided and also check the other algorithms, if we can find something we'll let you know but since this is not documented in any way it might be different from one version to another so kinda tricky to get the implementation right :(
Marian Edu

On 17 May 2019, at 11:00, ias wrote:

Thank you Marian.
I understand this. The problem is that it seems that ABL accepts IV of any length and, as it doesn't prepend it to the ciphertext it is difficult to understand what they do if e.g. the length of the provided IV is different from the one required by an algorithm. As a result we cannot guarantee the binary compatibility of our implementation of ENCRYPT/DECRYPT and ABL.
Regards,
Igor.

On 5/17/19 10:50 AM, Marian Edu wrote:

Igor,
those security-policy attributes are used in encrypt/decrypt only as defaults, one can always specify the key, vector, algorithm to both of those methods and indeed those have to match in order to be able to decrypt something that was encrypted but that the whole idea behind encryption I guess :)
We're completing the test cases now to cover the effects on encrypt/decrypt when any of those attributes are changed.
Other than that a memptr is just a pointer to a memory buffer and Progress has no knowledge that it's holding some encrypted data so no idea about what algorithm, key or vector has been used to encrypt it... it's just a bunch of bytes and nothing more, so if you try to decrypt that using the wrong key, vector or algorithm it is expected to not get the original unencrypted data :)
Cheers,
Marian Edu

In addition. The additional len parameter can be just ignored in blob and memptr which also implement writeByteRange.

Go ahead as proposed. A len == -1 can mean copy to the end of the data.

Greg Shah wrote:

Go ahead as proposed. A len == -1 can mean copy to the end of the data.

OK, thank you.

After fixing BinaryData.setString() and raw.writeByteRange the test is running.
I see some discrepancies at the end of the encrypted text and experienced problems with LargeObjectOps.copy (which corresponds to the COPY-LOB FROM decipher-raw TO decipher-text statement in my test. It throws ** Invalid character data found in MEMPTR for codepage ISO8859-15. (12012).
Continue investigation.

Both problems mentioned in note #53 were the result of appending zero when converting Text to raw in the SecurityOps.
Now the test passed for all algorithms with the same results as with 4GL when IV has the same length as required by an algorithm. There are still problems when it is not the case.
Continue investigation.

The converted test passed completely. The only difference is the result of the encryption when the provided IV is shorter than required by an algorithm. At this moment I do not see a fast way to figure how 4GL handles this case.

Igor Skornyakov wrote:

The only difference is the result of the encryption when the provided IV is shorter than required by an algorithm. At this moment I do not see a fast way to figure how 4GL handles this case.

You mean the IV's length is set to i.e 8 instead of 16, or that you set the length to 16, but only first 2 bytes have data?

Constantin Asofiei wrote:

You mean the IV's length is set to i.e 8 instead of 16, or that you set the length to 16, but only first 2 bytes have data?

At this moment I provide 16 bytes' IV with only the first 8 bytes filled with provided data. The JCE does not accept short IV.

The fixes committed to the branch 3810b rev. 11313.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

You mean the IV's length is set to i.e 8 instead of 16, or that you set the length to 16, but only first 2 bytes have data?

At this moment I provide 16 bytes' IV with only the first 8 bytes filled with provided data. The JCE does not accept short IV.

OK, so the algorithm requires 16 bytes IV. How does 4GL behave if you give a shorter IV?

Constantin Asofiei wrote:

OK, so the algorithm requires 16 bytes IV. How does 4GL behave if you give a shorter IV?

This is the question. I guess it uses some padding logic as we do, but obviously, it is not the same as ours.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

OK, so the algorithm requires 16 bytes IV. How does 4GL behave if you give a shorter IV?

This is the question. I guess it uses some padding logic as we do, but obviously, it is not the same as ours.

My assumption is that they would padded it with zeroes. Please test this in FWD and 4GL, and also the following: create some encrypted texts with 4GL (again, with combination of algorithm/iv/password/etc) and try to decrypt it in FWD. Is not enough that FWD can decrypt stuff that itself encrypted, we need to be able to decrypt stuff encrypted by 4GL.

It is possible to try to figure what exactly 4GL does with short IVs using "brute force attack". If the algorithm is more or less consisctent this should work. I will try it tomorrow.

Constantin Asofiei wrote:

My assumption is that they would padded it with zeroes. Please test this in FWD and 4GL, and also the following: create some encrypted texts with 4GL (again, with combination of algorithm/iv/password/etc) and try to decrypt it in FWD. Is not enough that FWD can decrypt stuff that itself encrypted, we need to be able to decrypt stuff encrypted by 4GL.

This assumption is wrong - we use padding with zeroes and get a different result. In my test, I print the encrypted data and the result is the same with p2j and 4GL. So we can "almost" be sure that our encryption is binary compatible with 4GL except for the case with short IV.

Igor Skornyakov wrote:

This assumption is wrong - we use padding with zeroes and get a different result. In my test, I print the encrypted data and the result is the same with p2j and 4GL. So we can "almost" be sure that our encryption is binary compatible with 4GL except for the case with short IV.

A simple test shows that if the IV length for the encrypt and decrypt differs, 4GL decrypts it fine (assuming the real data in the IV is only the first 2 bytes); they pad it with zeroes to match the length. You can go ahead to brute-force it, but I doubt you will find something else; please keep in mind the PUT-STRING 4GL issue I mentioned, to not get confused by random values.

The test I used is this - you can play with the iv-len and iv-len-decrypt parameters:

RUN CipherTest1(32, 2, 4, "AES_CBC_256").

PROCEDURE CipherTest1:
    DEF INPUT PARAMETER key-len AS INTEGER.
    DEF INPUT PARAMETER iv-len AS INTEGER.
    DEF INPUT PARAMETER iv-len-decrypt AS INTEGER.
    DEF INPUT PARAMETER alg AS CHAR.

    DEF VAR raw-iv AS RAW NO-UNDO.
    DEF VAR raw-iv1 AS RAW NO-UNDO.
    DEF VAR iv-hex AS CHAR NO-UNDO.
    DEF VAR raw-key AS RAW NO-UNDO.
    DEF VAR plain-text AS CHAR NO-UNDO.
    DEF VAR cipher-text AS MEMPTR NO-UNDO.
    DEF VAR cipher-raw AS RAW NO-UNDO.
    DEF VAR cipher-hex AS CHAR NO-UNDO.
    DEF VAR decipher-raw AS MEMPTR NO-UNDO.
    DEF VAR decipher-text AS LONGCHAR NO-UNDO.

    plain-text = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eius".

       IF iv-len > 0 THEN DO:
           LENGTH(raw-iv) = iv-len.
           PUT-STRING(raw-iv, 1) = "iv".
       END.
       ELSE
        raw-iv = ?.

    SECURITY-POLICY:SYMMETRIC-ENCRYPTION-IV = raw-iv.

    LENGTH(raw-key) = key-len.
    PUT-STRING(raw-key, 1) = "key".
    SECURITY-POLICY:SYMMETRIC-ENCRYPTION-KEY = raw-key.

    SECURITY-POLICY:SYMMETRIC-ENCRYPTION-ALGORITHM = alg.

    MESSAGE alg key-len iv-len.
    cipher-text = ENCRYPT(plain-text).

    cipher-raw = GET-BYTES(cipher-text, 1, GET-SIZE(cipher-text)).
    cipher-hex = HEX-ENCODE(cipher-raw).
    iv-hex = HEX-ENCODE(raw-iv).
    MESSAGE iv-hex.
    MESSAGE LENGTH(cipher-raw) cipher-hex.

          LENGTH(raw-iv1) = iv-len-decrypt.
          PUT-STRING(raw-iv1, 1) = "iv".
          SECURITY-POLICY:SYMMETRIC-ENCRYPTION-IV = raw-iv1.
       MESSAGE "IV updated".
          iv-hex = HEX-ENCODE(raw-iv1).
          MESSAGE iv-hex.

    decipher-raw = DECRYPT(cipher-text).
    COPY-LOB FROM decipher-raw TO decipher-text.
    IF plain-text = decipher-text THEN
        MESSAGE "OK".
    ELSE
        MESSAGE "FAIL".
END.

Since the encryption in the short IV case can still be decrypted, we can assume that the padding is not random.

There are some common approaches that might be used. For example:

• the valid provided bytes might be copied into the missing bytes (0x0102030405 might be turned into 0x01020304050102030405010203040501)
• some fixed padding character might be used (e.g. 0x20)
• the valid bytes might be overlaid on some standard/default IV such that the last bytes are some portion of the default value
• the input may be hashed into a 16-byte value using some standard algorithm such as ENCODE (it would be a terrible choice because of crytographic concerns, see #27)

The SECURITY-POLICY:SYMMETRIC-ENCRYPTION-IV is a readable attribute. What is the value when the session starts? When you assign it some value that is too short, then read the value back, do you get the padded value?

If those give no hints, we could try our own ideas above. Encrypt the value in the 4GL and try decrypting in FWD using different approaches to the IV.

Ultimately, we must get this to be a binary match because many applications may have pre-encrypted data which cannot be easily decrypted/reencrypted when switching to FWD... For example, if each user has their own key and only they can decrypt some data in the database (or elsewhere), then it is likely to be impractical to re-encrypt all data in the system.

Constantin Asofiei wrote:

A simple test shows that if the IV length for the encrypt and decrypt differs, 4GL decrypts it fine (assuming the real data in the IV is only the first 2 bytes); they pad it with zeroes to match the length. You can go ahead to brute-force it, but I doubt you will find something else; please keep in mind the PUT-STRING 4GL issue I mentioned, to not get confused by random values.

Well, this is a good argument, but it doesn't explain why our result is different despite the fact that we use padding with zero bytes. I will think about this tomorrow.

Greg Shah wrote:

The SECURITY-POLICY:SYMMETRIC-ENCRYPTION-IV is a readable attribute. What is the value when the session starts? When you assign it some value that is too short, then read the value back, do you get the padded value?

The default value is UNKNOWN. If set the returned value is the same which makes sense as the "right" legnth is different for different algoritms.

If those give no hints, we could try our own ideas above. Encrypt the value in the 4GL and try decrypting in FWD using different approaches to the IV.

Ultimately, we must get this to be a binary match because many applications may have pre-encrypted data which cannot be easily decrypted/reencrypted when switching to FWD... For example, if each user has their own key and only they can decrypt some data in the database (or elsewhere), then it is likely to be impractical to re-encrypt all data in the system.

This is what I plan to work on tomorrow.

I've found another issue. JCR doesn't support AES w/o IV. On encryption, it generates random IV if not provided, and rejects to decrypt w/o IV. Looking for a workaround.

Igor Skornyakov wrote:

I've found another issue. JCR doesn't support AES w/o IV. On encryption, it generates random IV if not provided, and rejects to decrypt w/o IV. Looking for a workaround.

In 4GL, encrypt something without setting the IV and try decrypting it with a fully-zero IV - maybe 4GL just fills the IV with zeroes if is not set.

Constantin Asofiei wrote:

In 4GL, encrypt something without setting the IV and try decrypting it with a fully-zero IV - maybe 4GL just fills the IV with zeroes if is not set.

It is a good idea, thank you. I will try.

Indeed, if the cipher requires IV but it is not provided, 4GL uses "all zeroes" IV. Thanks again to Constantin for a hint!

It appears that 4GL ignores the "length" of RAW when uses it as IV and, instead of padding just uses what it finds after "the end". I've added "cleaning" the variable used for IV before assigning a new value (by PUT-STRING(raw-iv, 16) = FILL(CHR(0), 16)), and now my test provides identical results with 4GL and P2J even for short IVs.
This means in particular that using short IVs is an extremely bad practice in 4GL as the real IV value is effectively unpredictable. We may encounter problems with the legacy code which uses such practice and I do not see any way to deal with it at this moment.

Branch 3810b updated. Rev. 11315. The uast/security/crypto.p test updated as well, revision 1854.

Maybe it is the right time to add a full implementation of GENERATE-PBE-KEY/GENERATE-PBE-SALT/MESSAGE-DIGEST as we have the support of the corresponding SECURITY-POLICY attributes?

GENERATE-PBE-KEY always generates 16 bytes using first 8 bytes of salt (if provided), presumably padding salt with zero bytes if it is shorter than 8.

It appears that 4GL ignores the "length" of RAW when uses it as IV and, instead of padding just uses what it finds after "the end". I've added "cleaning" the variable used for IV before assigning a new value (by PUT-STRING(raw-iv, 16) = FILL(CHR(0), 16)), and now my test provides identical results with 4GL and P2J even for short IVs.
This means in particular that using short IVs is an extremely bad practice in 4GL as the real IV value is effectively unpredictable. We may encounter problems with the legacy code which uses such practice and I do not see any way to deal with it at this moment.

Please test a case where the encrypt and decrypt are done in separate programs. These separate programs should be run in different operating system processes. In other words, run each of them with a separate command line. We don't know how the 4GL lays out its memory. It might be by order of variables in the program or by order of usage so something else. Try to affect the memory placement by having the two programs have different variables before and after the IV. Make sure those surrounding variables have different state. My idea is to try to prove that your short IV case is not safe (can fail in the 4GL).

Igor Skornyakov wrote:

It appears that 4GL ignores the "length" of RAW when uses it as IV and, instead of padding just uses what it finds after "the end". I've added "cleaning" the variable used for IV before assigning a new value (by PUT-STRING(raw-iv, 16) = FILL(CHR(0), 16)), and now my test provides identical results with 4GL and P2J even for short IVs.
This means in particular that using short IVs is an extremely bad practice in 4GL as the real IV value is effectively unpredictable. We may encounter problems with the legacy code which uses such practice and I do not see any way to deal with it at this moment.

It turns out the handling of IV is implemented based on the cipher key length, if provided IV raw contains less that the key length then the IV is zero padded and if it's greater than that whatever is beyond the key length is simply ignored. The key size for each algorithm is the last part in the cipher's name although you probably know the required size of the key on Java side. I've actually tested all sort of paddings like PCKS7 or ISO/IEC 7816-4 because using zero padding seemed not to work in our testing, the cause of that is still unknown but it seems to be related to the fact that we were doing the encryption in an internal procedure and there seems to. be some funny issue in 4GL that actually breaks the encryption and has something to do with the call stack as it only happens if the internal procedure had any kind of parameters... more tests in that one but I think it's just a Progress bug.

Igor Skornyakov wrote:

Maybe it is the right time to add a full implementation of GENERATE-PBE-KEY/GENERATE-PBE-SALT/MESSAGE-DIGEST as we have the support of the corresponding SECURITY-POLICY attributes?

Yes.

Greg Shah wrote:

Please test a case where the encrypt and decrypt are done in separate programs. These separate programs should be run in different operating system processes. In other words, run each of them with a separate command line. We don't know how the 4GL lays out its memory. It might be by order of variables in the program or by order of usage so something else. Try to affect the memory placement by having the two programs have different variables before and after the IV. Make sure those surrounding variables have different state. My idea is to try to prove that your short IV case is not safe (can fail in the 4GL).

OK.

Marian Edu wrote:

It turns out the handling of IV is implemented based on the cipher key length, if provided IV raw contains less that the key length then the IV is zero padded and if it's greater than that whatever is beyond the key length is simply ignored. The key size for each algorithm is the last part in the cipher's name although you probably know the required size of the key on Java side. I've actually tested all sort of paddings like PCKS7 or ISO/IEC 7816-4 because using zero padding seemed not to work in our testing, the cause of that is still unknown but it seems to be related to the fact that we were doing the encryption in an internal procedure and there seems to. be some funny issue in 4GL that actually breaks the encryption and has something to do with the call stack as it only happens if the internal procedure had any kind of parameters... more tests in that one but I think it's just a Progress bug.

Of course, the IV length depends on the algorithm (and mode). The result of my investigation is that 4GL does not perform any consistent padding if the provided IV is shorter than required. Regarding the cipher padding mode. My tests show that we get binary compatible results using NoPadding for CFB and OFB modes and RC4 and PKCS5Padding otherwise.

Contrary to what I wrote in note #75, the length of the key generated by GENERATE-PBE-KEY depends on the value of the SECURITY-POLICY:SYMMETRIC-ENCRYPTION-ALGORITHM attribute.

The 4GL code fragment

    DEF VAR raw-salt AS RAW NO-UNDO.
    LENGTH(raw-salt) = 1024.
    PUT-STRING(raw-salt, 1, 1024) = FILL(CHR(0), 1024).

runs ON in 4GL but causes an ArrayIndexOutOfBoundsException in P2j. Added additional limit check to raw.writeByteRange

Igor: Have you tried running the testcases mentioned in #3810-42? This is different from the testcases/uast/ directory. Please see the security/ directory for the tests related to this task.

Greg Shah wrote:

Igor: Have you tried running the testcases mentioned in #3810-42? This is different from the testcases/uast/ directory. Please see the security/ directory for the tests related to this task.

Greg, the test I see in the testcases/uast/security directory are related to MESSAGE-DIGEST, GUID, GENERATE-UUID, and CLIENT-PRINCIPAL. This is different from what I'm working on now.

I've implemented runtime support for GENERATE-PBE-KEY. In most cases, the results of running converted test are binary compatible with 4GL results but there is one problem.
I use Bouncy Castle PKCS5S1ParametersGenerator and it refuses to create keys which are longer than digest output (which is compliant with RFC 2898, see section 5.1). This is a problem for SHA-1/MD5 and "strong" ciphers with 24-byte keys.
Looking for a solution.

Igor Skornyakov wrote:

Greg Shah wrote:

Igor: Have you tried running the testcases mentioned in #3810-42? This is different from the testcases/uast/ directory. Please see the security/ directory for the tests related to this task.

Greg, the test I see in the testcases/uast/security directory are related to MESSAGE-DIGEST, GUID, GENERATE-UUID, and CLIENT-PRINCIPAL. This is different from what I'm working on now.

I'm not referencing the testcases/uast/security/. This is a different Testcases Project. Please follow this link and use that project.

Greg Shah wrote:

I'm not referencing the testcases/uast/security/. This is a different Testcases Project. Please follow this link and use that project.

Oh, I see. Sorry. Checking out...

Well, I will run these tests but at this moment they add nothing to my test. In addition, my test generates a detailed log which allows to easily compare the results of running it in 4GL and p2j.

If we want to generate a 24-byte key using MD5 the 4GL GENERATE-PBE-KEY generated 24 bytes where the first 16 bytes are exactly the same as max length key generated with PBKDF1 (RFC 2898). The nature of the remaining 8 bytes is unclear at this moment. The same situation with other "strong" keys generated with "weak" hash. Please note that this "tail" is consistent - if we generate the 32-byte key with MD5 the "tail" starts with the same 8 bytes.

PBKDF2 doesn't have the limitation on the generated key length regardless of the used hash but produces totally different results.

Added runtime support for GENERATE-PBE-KEY and GENERATE-PBE-SALT. Branch 3810b rev.113361.

Added uast/security/pbe-test.p test.

Still, have problems with generating "strong" keys with "weak" hashes (MD5 and SHA-1). See note #89.

The situation with GENERATE-PBE-KEY looks a little bit strange. Formally it "Generates a password-based encryption key, based on the PKCS#5/RFC 2898 standard". What "based on" means exactly remains unclear. RFC 2898 defines two PBE key derivation functions: PBKDF1 and PBKDF2. My tests show that 4GL for sure doesn't use PBKDF2 (see also https://knowledgebase.progress.com/articles/Article/000041878 where it is explicitly stated that "the OpenEdge GENERATE-PBE-KEY function is based on the PBKDF1 standard").
From the other side, PBKDF1 cannot generate an e.g. AES-256 key using MD5. but 4GL does provide an additional 16 bytes. I think that in this case, we have an example of "security by obscurity" and cryptography based on such keys cannot be considered as platform-independent.
Does it makes sense to try supporting binary compatibility for such "corner cases"?

Igor Skornyakov wrote:

The situation with GENERATE-PBE-KEY looks a little bit strange. Formally it "Generates a password-based encryption key, based on the PKCS#5/RFC 2898 standard". What "based on" means exactly remains unclear. RFC 2898 defines two PBE key derivation functions: PBKDF1 and PBKDF2. My tests show that 4GL for sure doesn't use PBKDF2 (see also https://knowledgebase.progress.com/articles/Article/000041878 where it is explicitly stated that "the OpenEdge GENERATE-PBE-KEY function is based on the PBKDF1 standard").
From the other side, PBKDF1 cannot generate an e.g. AES-256 key using MD5. but 4GL does provide an additional 16 bytes. I think that in this case, we have an example of "security by obscurity" and cryptography based on such keys cannot be considered as platform-independent.
Does it makes sense to try supporting binary compatibility for such "corner cases"?

Indeed, the 4GL always generate a key with a length required by the encryption algorithm used (security-policy:symmetric-encryption-algorithm) and that regardless of the hash algorithm used (security-policy:pbe-hash-algorithm). PBKDF1 states that the key length is bounded to the length of the hash output and at the time the RFC come out it was only MD2/MD5 and SHA-1(20), the 4GL does support also SHA-256(32) and SHA-512(64) as hash algorithm including for pbe-hash-algorithm so probably they made up their own 'extension' to the standard otherwise generate-pbe-key should have thrown an error when the key length required exceeded the length of the hash output - aka when pbe-hash-algorithm is MD5 and symmetric-encryption-algorithm requires a 32 byte key (aes_ecb_256).

The 4GL is known to be 'forgiven' in many cases so if you're looking for a binary compatible implementation you can maybe just enforce the PBKDF1 standard and throw an error as specified when the key length is longer than the hash output.

I thought they might be using a different hash algorithm to match the required key length regardless of what is set in pbe-hash-algoritm (like using sha-256 instead of md5 for 32 byte key) but this doesn't seem to be the case so how do they manage to generate 32 instead of 16 bytes is a mystery and certainly not a standard :(

Does it makes sense to try supporting binary compatibility for such "corner cases"?

On the one hand, I can see the value in generating a more cryptographically correct key instead of sometimes appending a "tail" that is fixed.

On the other hand, I worry that applications can have subtle dependencies upon this behavior such that they could rely upon it in some way.

I think we must match the output length and general behavior at a minimum. We may also want to duplicate the tail contents, but I'm not as sure about that.

Igor: What tests does Marian's team need to implement to explore this?

Greg Shah wrote:

On the one hand, I can see the value in generating a more cryptographically correct key instead of sometimes appending a "tail" that is fixed.

On the other hand, I worry that applications can have subtle dependencies upon this behavior such that they could rely upon it in some way.

I think we must match the output length and general behavior at a minimum. We may also want to duplicate the tail contents, but I'm not as sure about that.

Igor: What tests does Marian's team need to implement to explore this?

I suggest appending the short key with the beginning of the PBE key for the same password and salt which is -1L ^ salt, -1L if salt is undefined. This is not "better" than any other choice but it is consistent and simple.

Regarding the tests. My test (security/pbe-test.p) seems sufficient but of course, it looks ugly for a 4GL programmer. Maybe a more professional version of something like this will be nice to have. For me, its main advantage is a detailed log which allows me to compare the results of 4GL and P2J very fast.

As I see on the Internet people do discuss the problem of decrypting data encrypted by 4GL using other programming languages. However, they all discuss AES-128 where it is no problem with MD5 and SHA-1. I suspect that in most cases the default cipher and digest are used and if somebody decides to use a "strong" cipher then a "strong" hash is used as well, just in case. This makes me think that our problem is indeed a corner case. Maybe it makes sense to discuss it with our customers?

So far I cannot understand what AUDIT-POLICY:ENCRYPT-AUDIT-MAC-KEY actually does. What I see is that it returns a hex encoded byte array of the same length as its argument. Moreover if key2.startsWith(key1) then ENCRYPT-AUDIT-MAC-KEY(key2).startsWith(ENCRYPT-AUDIT-MAC-KEY(key1)). The result doesn't depend either on SECURITY-POLICY:SYMMETRIC-ENCRYPTION-ALGORITHM or SECURITY-POLICY:PBE-HASH-ALGORITHM

Maybe it makes sense to discuss it with our customers?

At this point we are trying to finish this work permanently instead of deferring it until later. Go ahead and try to match the behavior as seen by the 4GL code. This seems to me to be the length of the result and the fixed nature of the tail bytes. Is there anything else that can be seen by the 4GL code as different?

Greg Shah wrote:

At this point we are trying to finish this work permanently instead of deferring it until later. Go ahead and try to match the behavior as seen by the 4GL code. This seems to me to be the length of the result and the fixed nature of the tail bytes. Is there anything else that can be seen by the 4GL code as different?

OK. It seems that I was not clear enough. The tail bytes are not fixed - they depend both on password and salt. However, it looks that e.g. the tail of the 24 bytes generated key is the same as first 8 bytes of the tail of the 32 bytes key if the password and salt are the same. This was the motivation of my suggestion in note #94.

Apart from the "tail problem" the current version of the p2j runtime produces the same result as 4GL.

I've added the tail generation for the GENERATE-PBE-KEY. The tail is generated using the first 8 bytes of the PBKDF1 key as a salt for the next iteration.
Committed to the branch 3810b revision 11317.

Do we need to provide binary compatibility for the AUDIT-POLICY:ENCRYPT-AUDIT-MAC-KEY (see note #96)? Or it is sufficient e.g. to encrypt the argument with a stream cipher (such as RC4) with some hard-coded(?) key?

Well, it seems that ENCRYPT-AUDIT-MAC-KEY is something very simple. A quote from https://community.progress.com/community_groups/openedge_development/f/19/t/8627

It is not salted.  It is not secure.  It is trivially breakable.  It protects data from casual snooping, nothing more.  It is the same method used by genpassword to produce "encrypted" passwords ("oech1::blahblah").  The code is implemented in a PSC Java class (com.progress.common.util.genPassword), if you're interested to see what it does.  

I've added the tail generation for the GENERATE-PBE-KEY. The tail is generated using the first 8 bytes of the PBKDF1 key as a salt for the next iteration.

Are there any remaining deviations from the 4GL approach or is this compatible at this point?

Well, it seems that ENCRYPT-AUDIT-MAC-KEY is something very simple.

I guess that make it easier to implement in a compatible way. Please match their approach.

I've added the tail generation for the GENERATE-PBE-KEY. The tail is generated using the first 8 bytes of the PBKDF1 key as a salt for the next iteration.

Are there any remaining deviations from the 4GL approach or is this compatible at this point?

The P2J tail is different from one generated by 4GL. This affects 24- and 32-bytes keys with MD5 and SHA-1. I do not see how it is possible to figure the algorithm used by 4GL.

Well, it seems that ENCRYPT-AUDIT-MAC-KEY is something very simple.

I guess that make it easier to implement in a compatible way. Please match their approach.

I understand that we do not use reverse engineering, so the analysis of the com.progress.common.util.genPassword code is not an option. I will try to understand their algorithm in another way.

The P2J tail is different from one generated by 4GL. This affects 24- and 32-bytes keys with MD5 and SHA-1. I do not see how it is possible to figure the algorithm used by 4GL.

Do we generate the same length result as the 4GL in all cases?

What differences can be detected by 4GL code?

I understand that we do not use reverse engineering, so the analysis of the com.progress.common.util.genPassword code is not an option. I will try to understand their algorithm in another way.

Correct. Good.

Greg Shah wrote:

The P2J tail is different from one generated by 4GL. This affects 24- and 32-bytes keys with MD5 and SHA-1. I do not see how it is possible to figure the algorithm used by 4GL.

Do we generate the same length result as the 4GL in all cases?

Yes, we do.

Well, the AUDIT-POLICY:ENCRYPT-AUDIT-MAC-KEY algorithm is indeed very simple.
1. Using a long string of identical characters one can easily see that it actually works with 8 bytes' blocks
2. I've already mentioned (note #98) that the if key2.startsWith(key1) then ENCRYPT-AUDIT-MAC-KEY(key2).startsWith(ENCRYPT-AUDIT-MAC-KEY(key1))
3. It is easy to check those bytes before the given byte also does not affect the encoding of this byte.

This means that we have a variation of the ancient substitution cipher where the substitution depends only on the byte and its position.
The first guess about XOR appeared to be correct and the "key" is "PROGRESS".

Now the runtime support for the AUDIT-POLICY:ENCRYPT-AUDIT-MAC-KEY is binary compatible with 4GL.
Committed to the branch 3810b rev. 11318.

The corresponding test is teastcases/uast/security/mac-test.p

Well done!

As we finished with cryptography may be it makes sense to merge 3810b and do the rest in the 3810c?

Code Review Task Branch 3810b Revision 11318

1. In AuditPolicyManager, SecurityOps, SecurityPolicyManager, CryptoUtils the use of com.google.common.* brings in a dependency on Guava which I prefer to avoid. Previously it was only in the admin GWT usage, but I see it has recently "escaped" into the tree-list and environment ops. We will address those other cases separately, but let's remove this one now.

2. In SecurityOps.generatePBESalt(), I don't see the code that fills the array with a random value.

3. In SecurityOps.encryptDecryptWorker() the error for unknown cipher algorithm should be replaced with the error that would occur in the 4GL for this case.

4. In memptr.writeByteRange(), I'm worried that the addition of the len parameter creates a contract that we (intentionally) do not honor. The problem is that future readers of that code (or just of the javadoc) can't know this. If we aren't going to honor it at the client API, then we probably need javadoc and a WARNING comment in that method.

5. The branch needs a rebase.

6. Please review the implementation so far and make a list of the places where we need tests to show error behavior. I know you have unknown value behavior tested and implemented in most (or all?) of the new features. I'm concerned about the cases where a value is passed but it has some unexpected properties (e.g. a negative number for something that should be positive or a number that is out of a valid range). Marian's team can make sure we have these cases covered in their tests.

7. CryptoUtils.CipherParams (and the CryptoUtils.test() and CryptoUtils.testAll() methods) need javadoc.

8. Please recall that we use wildcard imports unless there is a an explicit conflict or other overriding reason. See AuditPolicyManager, SecurityOps, SecurityPolicyManager, CryptoUtils.

I've just noticed that albeit 4GL validation of the value of the SECURITY-POLICY:SYMMETRIC-ENCRYPTION-ALGORITHM is case-insensitive (for example it accepts "aES_CBC_256" as valid, the "algorithm" argument of the ENCRYPT/DECRYPT is case sensitive. In particular, if the SECURITY-POLICY:SYMMETRIC-ENCRYPTION-ALGORITHM was set to "aES_CBC_256" the ENCRYPT with the default "algorithm" argument will fail with error 12146.

Greg Shah wrote:

Code Review Task Branch 3810b Revision 11318

1. In AuditPolicyManager, SecurityOps, SecurityPolicyManager, CryptoUtils the use of com.google.common.* brings in a dependency on Guava which I prefer to avoid. Previously it was only in the admin GWT usage, but I see it has recently "escaped" into the tree-list and environment ops. We will address those other cases separately, but let's remove this one now.

Done.

2. In SecurityOps.generatePBESalt(), I don't see the code that fills the array with a random value.

Fixed.

3. In SecurityOps.encryptDecryptWorker() the error for unknown cipher algorithm should be replaced with the error that would occur in the 4GL for this case.

Fixed.

4. In memptr.writeByteRange(), I'm worried that the addition of the len parameter creates a contract that we (intentionally) do not honor. The problem is that future readers of that code (or just of the javadoc) can't know this. If we aren't going to honor it at the client API, then we probably need javadoc and a WARNING comment in that method.

Sorry, I do not understand the point. The len parameter was added because of the ArrayIndexBoundException was thrown in some cases which runs OK with 4GL, and the result of the PUT-STRING(raw, pos, len) = string could be incorrect.
Do you mean that it was done intentionally?

5. The branch needs a rebase.

Done. Rev. 11322.

6. Please review the implementation so far and make a list of the places where we need tests to show error behavior. I know you have unknown value behavior tested and implemented in most (or all?) of the new features. I'm concerned about the cases where a value is passed but it has some unexpected properties (e.g. a negative number for something that should be positive or a number that is out of a valid range). Marian's team can make sure we have these cases covered in their tests.

I've tested all the situations I could imagine including UNKNOWN values. I hope that Marian's team will find what I overlooked.

7. CryptoUtils.CipherParams (and the CryptoUtils.test() and CryptoUtils.testAll() methods) need javadoc.

Fixed.

8. Please recall that we use wildcard imports unless there is a an explicit conflict or other overriding reason. See AuditPolicyManager, SecurityOps, SecurityPolicyManager, CryptoUtils.

Fixed. Sorry, I forgot about this. I've adjusted my IDE setting to avoid this problem in the future.

The len parameter was added because of the ArrayIndexBoundException was thrown in some cases which runs OK with 4GL, and the result of the PUT-STRING = string could be incorrect.
Do you mean that it was done intentionally?

No, I mean that it is intentional that we are ignoring the len in the memptr.writeByteRange(). However, the reader of that javadoc or code would not know this. This means someone later on could read the javadoc and thing it is valid to specify a len that is NOT -1 to the memptr.writeByteRange(). Eventually they might find that it doesn't do what they expect and think this is a bug. In fact, we intentionally ignore len there so we should make that explicit in the code and javadoc.

Greg Shah wrote:

No, I mean that it is intentional that we are ignoring the len in the memptr.writeByteRange(). However, the reader of that javadoc or code would not know this. This means someone later on could read the javadoc and thing it is valid to specify a len that is NOT -1 to the memptr.writeByteRange(). Eventually they might find that it doesn't do what they expect and think this is a bug. In fact, we intentionally ignore len there so we should make that explicit in the code and javadoc.

Got it. memptr.writeBytesRange() Javadoc fixed

Igor Skornyakov wrote:

Greg Shah wrote:

No, I mean that it is intentional that we are ignoring the len in the memptr.writeByteRange(). However, the reader of that javadoc or code would not know this. This means someone later on could read the javadoc and thing it is valid to specify a len that is NOT -1 to the memptr.writeByteRange(). Eventually they might find that it doesn't do what they expect and think this is a bug. In fact, we intentionally ignore len there so we should make that explicit in the code and javadoc.

Got it. memptr.writeBytesRange() Javadoc fixed

I probably don't know the details here but in 4GL there is a difference between memptr and raw variables. The memptr needs to be allocated with at least the size of the bytes that are written while for raw Progress will autoresize it if needed, there is a catch however as it will only resize it up and unless reset (length set to 0) adding a shorter string into it doesn't make it shrink. But this is expected behavior and although it might look like there is an issue with the encrypt/decrypt in fact those works just fine - using all raw bytes, there can be anything not just a string.

put-string(rw, 1) = 'long' => length is 5, one extra chr(0) at the end
put-string(rw, 1, 2) = 'iv' => length is still 5, there is a chr(0) at offset 3 but then on 4th byte there is a 'g' :)

This is not because the last, optional, size parameter is being ignored in any way and in 4GL this will not reduce the size of the variable... the developer must take care of that if reusing a longchar/memptr variable.

Using set-size on a memptr that is already allocated doesn't have any effect, unless first reset to zero. Using length on a raw variable will work both for increasing/decreasing the size - the data in it remains as-is, null bytes are appended at the end if size is increased.

Marian Edu wrote:

I probably don't know the details here but in 4GL there is a difference between memptr and raw variables. The memptr needs to be allocated with at least the size of the bytes that are written while for raw Progress will autoresize it if needed, there is a catch however as it will only resize it up and unless reset (length set to 0) adding a shorter string into it doesn't make it shrink. But this is expected behavior and although it might look like there is an issue with the encrypt/decrypt in fact those works just fine - using all raw bytes, there can be anything not just a string.

put-string(rw, 1) = 'long' => length is 5, one extra chr(0) at the end
put-string(rw, 1, 2) = 'iv' => length is still 5, there is a chr(0) at offset 3 but then on 4th byte there is a 'g' :)

This is not because the last, optional, size parameter is being ignored in any way and in 4GL this will not reduce the size of the variable... the developer must take care of that if reusing a longchar/memptr variable.

Using set-size on a memptr that is already allocated doesn't have any effect, unless first reset to zero. Using length on a raw variable will work both for increasing/decreasing the size - the data in it remains as-is, null bytes are appended at the end if size is increased.

Marian,
The issue we discuss here is that P2J runtime ignored the len argument of the PUT-STRING which is both incompatible with 4GL behavior (as I understand it) and can result in the ArrayIndexBoundException.

Marian, thank you for the notes.

I believe that at the external interface for RAW and MEMPTR, we do implement as you've noted. The writeBytesRange() method is a low level worker and the caller must have already done the proper bounds checking, auto-expanding (raw only) and error generation if there is anything wrong. The key here is that we cannot allow actual access to invalid memory or ArrayIndexOutOfBounds (for RAW which we implement with a byte[]). So we check these conditions before trying the access.

My comments to Igor were related to making sure that future Java callers of that code would know the proper contract. In this case we don't honor the len value for the memptr version of this API. The reason is that we just recently added this parameter for other users (not memptr) of the API and instead of adding full support memptr we are just leaving the current behavior and ignoring len. As long as the javadoc is correct, this is OK.

It seems that we have a problem with the conversion of the CREATE CLIENT-PRINCIPAL statement. The following snippet

DEF VAR cp AS HANDLE NO-UNDO.
CREATE CLIENT-PRINCIPAL cp.

      handle cp = TypeFactory.handle();

      externalProcedure(ClientPrincipal.this, new Block((Body) () -> 
      {
         ClientPrincipal.create(cp);
      }));

which results in the compilation error.

Igor Skornyakov wrote:

It seems that we have a problem with the conversion of the CREATE CLIENT-PRINCIPAL statement. The following snippet
[...]
is converted tp
[...]

which results in the compilation error.

Is your program named client_principal.p? If so, please rename the program to something else.

Constantin Asofiei wrote:

Is your program named client_principal.p? If so, please rename the program to something else.

Oh, I see. Thanks a lot!

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Is your program named client_principal.p? If so, please rename the program to something else.

Oh, I see. Thanks a lot!

Unfortunately, after renaming the result is the same.

Igor Skornyakov wrote:

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Is your program named client_principal.p? If so, please rename the program to something else.

Oh, I see. Thanks a lot!

Unfortunately, after renaming the result is the same.

What is the new name? clientPrincipal.p? Rename it to i.e. clientp.p and make sure to delete the previous ClientPrincipal.java source.

Constantin Asofiei wrote:

What is the new name? clientPrincipal.p? Rename it to i.e. clientp.p and make sure to delete the previous ClientPrincipal.java source.

The name is client-principal-test1.p

Igor Skornyakov wrote:

Constantin Asofiei wrote:

What is the new name? clientPrincipal.p? Rename it to i.e. clientp.p and make sure to delete the previous ClientPrincipal.java source.

The name is client-principal-test1.p

This is really strange - the method ClientPrincipal.create(handle h) exists but there is a compilation error. Both with javac and Eclipse. I will take a look tomorrow with a "fresh head".

In multiple runs of the regression testing, the following tests consistently fail:

gso_29
gso_185
gso_190
gso_255
gso_265
gso_273
gso_278
gso_319
gso_375
gso385_session1
gso385_session2
gso_394
gso_482

tc_item_master_005
tc_item_master_028
tc_item_master_082
tc_item_master_006
tc_item_master_104

They also fail when running with run_single.sh

I've created a fresh branch from the trunk (3810c) and most of these tests failed as well.

The CTRL-C tests passed in one on runs but starting from Sunday they just hang.

Any suggestions?

Added runtime support for the CLIENT-PRINCIPAL new attributes. SEAL method updated to use these attributes.

Branch 3810b rev. 11323.

See also uast/security/client-principal-test.p and uast/security/client-principal-test1.p

Branch 3810b rebased to the trunk rev. 11311.
Committed revision 11327

Finished implementation and testing runtime support for all new attributes and methods of the CLIENT-PRINCIPAL except VALIDATE-SEAL, GET-DB-CLIENT, SET-DB-CLIENT. Updated and tested CLIENT-PRINCIPAL:SEAL method.

Branch 3810b rev. 11325

Used testcases/uast/security/cp-test.p for testing.

Igor Skornyakov wrote:

Branch 3810b rebased to the trunk rev. 11311.
Committed revision 11327

Your rebase is wrong... what commands did you use?

• checkout a fresh FWD trunk
• make sure everything is committed in your 3810b branch
• go to 3810b branch a do:
  • bzr unbind
  • bzr rebase path/to/p2jtrunk/
  • solve collisions, use bzr resolve <filename> and bzr rebase-continue
  • when finished, do bzr push --overwrite ~/secure/code/p2j_repo/p2j/trunk
  • bzr bind

Constantin Asofiei wrote:

Your rebase is wrong... what commands did you use?

The steps are these:

• checkout a fresh FWD trunk
• make sure everything is committed in your 3810b branch
• go to 3810b branch a do:
  • bzr unbind
  • bzr rebase path/to/p2jtrunk/
  • solve collisions, use bzr resolve <filename> and bzr rebase-continue
  • when finished, do bzr push --overwrite ~/secure/code/p2j_repo/p2j/trunk
  • bzr bind

Thank you Constantin, but I did not mean to update the trunk as it was not approved. Moreover, I have problems with regression testing - see note #124.

Igor Skornyakov wrote:

Thank you Constantin, but I did not mean to update the trunk as it was not approved.

Sorry, the last command was meant to target 3810b, not trunk, like bzr push --overwrite ~/secure/code/p2j_repo/p2j/active/3810b. Anyway, as I mentioned, the rebase is wrong, you can do a diff between the current version and last trunk version, as you did a trunk merge into 3810b, I think.

Moreover, I have problems with regression testing - see note #124.

Please run again, and see which fail consistently.

Constantin Asofiei wrote:

Igor Skornyakov wrote:

Thank you Constantin, but I did not mean to update the trunk as it was not approved.

Sorry, the last command was meant to target 3810b, not trunk, like bzr push --overwrite ~/secure/code/p2j_repo/p2j/active/3810b. Anyway, as I mentioned, the rebase is wrong, you can do a diff between the current version and last trunk version, as you did a trunk merge into 3810b, I think.

Thank you, I will double check.

Moreover, I have problems with regression testing - see note #124.

Please run again, and see which fail consistently.

I've run 3 times and with run_single and the tests fail. Moreover, as I've mentioned in note #124 they fail with the trunk as well.

There are some weird errors in your setup, for example server.log has:

[05/27/2019 08:25:54 EDT] (com.goldencode.p2j.persist.dialect.P2JPostgreSQLDialect:INFO) Old rtrim: [upper(rtrim(check_type, '
^M'::text))].

And your postgres log has messages like this:

2019-05-27 08:26:04 EDT [26783-1] gc@rfq_20090909_staging FATAL:  sorry, too many clients already

• shutdown the FWD servers via run_regression.sh server-shutdown
• execute a run_regression.sh server-startup to start servers and restore master DBs
• shutdown the FWD servers via run_regression.sh server-shutdown
• reindex the DBs via these commands, while in the testing/ folder:
  

  psql -U gc -p 5441 -f majic/ddl/schema_index_majic_postgresql.sql gso_20090909_staging
  psql -U gc -p 5441 -f majic/ddl/schema_index_majic_postgresql.sql rfq_20090909_staging
  

• run the main part via run_regressions.sh main-regression -Ddb.restore=no

If main part is better now, then you can re-index the master DBs directly.

Constantin Asofiei wrote:

There are some weird errors in your setup, for example server.log has:
[...]
which shouldn't be there.

And your postgres log has messages like this:
[...]
I think there is something wrong with your master DB.

Try this:

• shutdown the FWD servers via run_regression.sh server-shutdown
• execute a run_regression.sh server-startup to start servers and restore master DBs
• shutdown the FWD servers via run_regression.sh server-shutdown
• reindex the DBs via these commands, while in the testing/ folder:
  [...]
• run the main part via run_regressions.sh main-regression -Ddb.restore=no

If main part is better now, then you can re-index the master DBs directly.

I see. Thanks a lot!

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Igor Skornyakov wrote:

Thank you Constantin, but I did not mean to update the trunk as it was not approved.

Sorry, the last command was meant to target 3810b, not trunk, like bzr push --overwrite ~/secure/code/p2j_repo/p2j/active/3810b. Anyway, as I mentioned, the rebase is wrong, you can do a diff between the current version and last trunk version, as you did a trunk merge into 3810b, I think.

Thank you, I will double check.

It seems that 3810b is in a normal shape. When I try to rebase it I get "No revisions to rebase". And the differences with the trunk look as expected.

Igor Skornyakov wrote:

It seems that 3810b is in a normal shape. When I try to rebase it I get "No revisions to rebase". And the differences with the trunk look as expected.

That's not what I mean - I assume you did a diff between 3810b latest rev and a separate trunk checkout?

bzr log -r 11311 in 3810b should give you the same log message as bzr log -r 11311 in a trunk checkout. And a bzr diff -r 11311 in 3810b should give you only changes part of 3810b. That's what I mean the rebase is wrong. After rebase, 3810b should have the revisions up to 11311 be the same as the trunk checkout, and from there on 3810b specific revisions.

If the latest 3810b is OK (and it contains your correct changes), I think we will go ahead and diff 3810b with a trunk checkout, as I don't think is that easy to fix 3810b. What commands did you use for the rebase?

Constantin Asofiei wrote:

If the latest 3810b is OK (and it contains your correct changes), I think we will go ahead and diff 3810b with a trunk checkout, as I don't think is that easy to fix 3810b. What commands did you use for the rebase?

I use the same commands you describe except bzr push. Sorry for this fault.

Constantin Asofiei wrote:

There are some weird errors in your setup, for example server.log has:

• run the main part via run_regressions.sh main-regression -Ddb.restore=no

If main part is better now, then you can re-index the master DBs directly.

Constantin.
Now it is much better - only 3 tc tests failed (tc_po_item_018, tc_job_002, tc_job_clock_002 - as far as I remember these tests fail pretty often).
How can I reindex master DBs. Sorry for my ignorance but I've never done it before and cannot find the corresponding instructions.
Thank you.

Igor Skornyakov wrote:

Now it is much better - only 3 tc tests failed (tc_po_item_018, tc_job_002, tc_job_clock_002 - as far as I remember these tests fail pretty often).

Great, then the reason was a missed update on the master DBs.

How can I reindex master DBs. Sorry for my ignorance but I've never done it before and cannot find the corresponding instructions.

No problem. Just use gso_20090909_2_29_3a_master and rfq_20090909_2_29_3a_master as the DBs instead of the staging DBs, in the psql commands.

Constantin Asofiei wrote:

No problem. Just use gso_20090909_2_29_3a_master and rfq_20090909_2_29_3a_master as the DBs instead of the staging DBs, in the psql commands.

Thanks a lot!

After the re-indexing of DBs the only test which fails in multiple runs of the ChUI regression testing of the 3810b is tc_job_002.

Marian,
Can you please help me to create a simple test with the session registry which supports authentication (I understand it should be created via SECURITY-POLICY:LOAD-DOMAINS). Without having such an example it will be difficult for me to implement correct support for e.g. SECURITY-POLICY:SET-CLIENT.
Thank you,
Igor.

Igor Skornyakov wrote:

After the re-indexing of DBs the only test which fails in multiple runs of the ChUI regression testing of the 3810b is tc_job_002.

That is PASSED since that test is expected to fail.

Greg Shah wrote:

That is PASSED since that test is expected to fail.

Oh, sorry - I remember that this test is special but forgot that it is supposed to fail.
Thank you.

It seems that we have no support for the CLIENT-PRINCIPAL:INITIALIZE method. There is no even INITIALIZE keyword in progress.g. Was it done intentionally? I think it makes sense to support it.
Any thoughts?

Igor Skornyakov wrote:

It seems that we have no support for the CLIENT-PRINCIPAL:INITIALIZE method. There is no even INITIALIZE keyword in progress.g. Was it done intentionally? I think it makes sense to support it.
Any thoughts?

Please add it.

Constantin Asofiei wrote:

There are some weird errors in your setup, for example server.log has:

[05/27/2019 08:25:54 EDT] (com.goldencode.p2j.persist.dialect.P2JPostgreSQLDialect:INFO) Old rtrim: [upper(rtrim(check_type, '^M'::text))].

which shouldn't be there.

I am certain that after refreshing the database (more precise UDFs & computed columns) these messages have disappeared. Just to let you know: some time ago (2 years maybe) FWD used an incorrect trimming function. It was corrected but, because the internal processing of computed columns a save mechanism was kept for an undertermined transition period.
It's strange that you encountered such old database. However, as noted before, refreshing the database should remove these warning messages.

Test cases for security related functions are in 'security' folder in bazaar repository, below what we've tested so far:

AUDIT-CONTROL system handle¶

1. Attributes
• APPL-CONTEXT-ID – char – RO
• EVENT-GROUP-ID – char – RO
• HANDLE – handle – RO
• INSTANTIATING-PROCEDURE – handle – RO
• TYPE – char – RO
1. Methods
• BEGIN-EVENT-GROUP
• LOG-AUDIT-EVENT
• SET-APPL-CONTEXT

AUDIT-POLICY system handle¶

1. Attributes
• HANDLE – handle – RO
• INSTANTIATING-PROCEDURE – handle – RO
• TYPE – char – RO
1. Methods
• ENCRYPT-AUDIT-MAC-KEY – test generated value for different encrypt-key lengths
• REFRESH-AUDIT-POLICY

SECURITY-POLICY system handle¶

1. Attributes
• CAN-DO-DOMAIN-SUPPORT – logical – RW
• ENCRYPTION-SALT – raw – RW
• HANDLE – handle – RO
• INSTANTIATING-PROCEDURE – handle – RO
• PBE-HASH-ALGORITHM – char – RW

• Test behavior of GENERATE-PBE-KEY when this attribute is written with combinations of lower and upper case

• PBE-KEY-ROUNDS – integer – RW
• SYMMETRIC-ENCRYPTION-ALGORITHM – char – RW

• Test behavior of ENCRYPT when this attribute is written with combinations of lower and upper case

• SYMMETRIC-ENCRYPTION-IV – raw – RW
• SYMMETRIC-ENCRYPTION-KEY – raw WO
• SYMMETRIC-SUPPORT – char – RO
• TYPE – char – RO
• XCODE-SESSION-KEY – char WO
1. Methods
• GET-CLIENT
• LOAD-DOMAINS
• LOCK-REGISTRATION
• REGISTER-DOMAIN
• SET-CLIENT
• GENERATE-PBE-KEY behavior

• Test generated key at byte level for equality with keys generated in separate files using ALL combinations of:

• SYMMETRIC-ENCRYPTION-ALGORITHM – 21 values
• PBE-HASH-ALGORITHM – 4 values
• PBE-KEY-ROUNDS – 2 values ( 100 ; 1000 )
• ENCRYPTION-SALT – 2 values ( 4 bytes ; 16 bytes )

• Test generated key using SYMMETRIC-ENCRYPTION-IV in following mode

• Use SYMMETRIC-ENCRYPTION-IV with length lower than GENERATE-PBE-KEY – as is AND completed with 0
• Use SYMMETRIC-ENCRYPTION-IV with length greater than GENERATE-PBE-KEY – as is AND truncated
• Test generated key using ENCRYPTION-SALT in following mode
• Use ENCRYPTION-SALT with length lower than 8 – as is AND completed with 0
• Use ENCRYPTION-SALT with length greater than 8 – as is AND truncated

• Test key length against SYMMETRIC-ENCRYPTION-ALGORITHM length using ALL combinations of:

• SYMMETRIC-ENCRYPTION-ALGORITHM – 21 values
• PBE-HASH-ALGORITHM – 4 values
• ENCRYPTION-SALT – 2 values ( 4 bytes ; 18 bytes )
• Password – 2 values ( 3 char ; 34 char )

• ENCRYPT behavior

• Test raw generated using SYMMETRIC-ENCRYPTION-IV in following mode

• Use SYMMETRIC-ENCRYPTION-IV with length lower than GENERATE-PBE-KEY – as is AND completed with 0
• Use SYMMETRIC-ENCRYPTION-IV with length greater than GENERATE-PBE-KEY – as is AND truncated
• Use all SYMMETRIC-ENCRYPTION-ALGORITHM – 21 values

• DECRYPT behavior

• Generate file with encrypted text in following mode:

• SYMMETRIC-ENCRYPTION-ALGORITHM – 21 values
• PBE-HASH-ALGORITHM – 4 values
• Constant values for:

• ENCRYPTION-SALT
• PBE-KEY-ROUNDS
• SYMMETRIC-ENCRYPTION-IV
• Password
• Text to Encrypt

• Decrypt each file and compare result for identical values

Marian Edu wrote:

Test cases for security related functions are in 'security' folder in bazaar repository, below what we've tested so far:

Thank you very much Marian!

Marian,
The get-client.p test fails in my runs. I understand that this happens because I've not provided the "fwd" DB. Can you please advise me how to do this? I did something similar in the past but completely forgot the details. Sorry for my ignorance.
Thank you,
Igor.

Igor Skornyakov wrote:

Marian,
The get-client.p test fails in my runs. I understand that this happens because I've not provided the "fwd" DB. Can you please advise me how to do this? I did something similar in the past but completely forgot the details. Sorry for my ignorance.
Thank you,
Igor.

Hi Igor,

not sure how you can do that in FWD but in Progress we have a 'fwd' database where we've added one entry in the users table (system table) and that is used for client principal validation with security-policy:set-client or set-db-client function.

There is a database schema definition (fwd.df) along with the users table export (user.d) in 'db' folder. Don't know how you can load that users table in FWD but there is really just one entry there: user name is 'fwd' and password is also 'fwd'.

Marian Edu wrote:

Hi Igor,

not sure how you can do that in FWD but in Progress we have a 'fwd' database where we've added one entry in the users table (system table) and that is used for client principal validation with security-policy:set-client or set-db-client function.

There is a database schema definition (fwd.df) along with the users table export (user.d) in 'db' folder. Don't know how you can load that users table in FWD but there is really just one entry there: user name is 'fwd' and password is also 'fwd'.

Marian,
My question was about Progress. How can a create and populate fwd database using e.r. pro? I'm looking in the documentation but it is not clear so far.
Sorry again for a stupid question.
Thank you,
Igor.

Marian Edu wrote:

If you don't have the database created you can do it from there as well - in data dictionary create a database, name it 'fwd' and then you can load the definition from 'db/fwd.df' and in the end load the users.

Thank you very much Marian!

Does FWD have standard support for a user table?
Thank you.

It supports the _user table. I assume that is what is meant.

Greg Shah wrote:

It supports the _user table. I assume that is what is meant.

Yes, I meant _user. Thank you.

How can we force generation new character() as the method argument value if a ? literal is provided in the 4GL code?
Thank you.

Search rules/ for "wrap" to see how this annotation will cause unknown value and primitive types to be emitted as BaseDataType instances.

Greg Shah wrote:

Search rules/ for "wrap" to see how this annotation will cause unknown value and primitive types to be emitted as BaseDataType instances.

Thank you!

Igor, before your next merge, please check/fix javadoc issues related to the 3810 code. Thanks.

On 6/4/19 7:45 PM, ias wrote:

Gentlemen,
Which is the right ErrorManager method to be invoked to show error message in the dialog box, write it to the file if output is redirected, and continue program execution? Is it displayErrorRedirected?
Thank you,
Igor.

Is an ERROR condition raised? Are you trying to fix an argument validation error?

Usually we are using recordOrThrowError (if an ERROR condition is raised) and recordOrShowError, otherwise.

Use the NO-ERROR option and check the ERROR-STATUS:ERROR, ERROR-STATUS:NUM-MESSAGES and ERROR-STATUS:GET-MESSAGE, to know what kind of message you need to log; if ERROR flag is set, then recordOrThrowError is required.

Another approach is to use a block like this:

do on error undo, leave:

  // code you want to test.

  message "if here, no ERROR condition is raised".

end.

If the MESSAGE is reached, then ERROR condition is not raised.

Constantin Asofiei wrote:

Is an ERROR condition raised? Are you trying to fix an argument validation error?

Usually we are using recordOrThrowError (if an ERROR condition is raised) and recordOrShowError, otherwise.

Use the NO-ERROR option and check the ERROR-STATUS:ERROR, ERROR-STATUS:NUM-MESSAGES and ERROR-STATUS:GET-MESSAGE, to know what kind of message you need to log; if ERROR flag is set, then recordOrThrowError is required.

Another approach is to use a block like this:
[...]

If the MESSAGE is reached, then ERROR condition is not raised.

Thank you Constantin. My question was about FWD Java code. I use 4GL test which redirects output to a file to check if my runtime support produces the same results. I've found in particular that some parts of the ClientPrincipal (both existing and mine) use recordOrShowError which aborts the test.

Igor Skornyakov wrote:

Thank you Constantin. My question was about FWD Java code. I use 4GL test which redirects output to a file to check if my runtime support produces the same results. I've found in particular that some parts of the ClientPrincipal (both existing and mine) use recordOrShowError which aborts the test.

Please post a 4GL snippet so I can better understand this.

Constantin Asofiei wrote:

Please post a 4GL snippet so I can better understand this.

Well, for example, if the CLIENT-PRINCIPAL is expired, the statement

MESSAGE "SEAL" hcp:SEAL("access-code") hcp:SEAL-TIMESTAMP "STATE-DETAIL" "LOGIN-STATE" hcp:LOGIN-STATE hcp:STATE-DETAIL.

CLIENT-PRINCIPAL:SEAL failed because object expired (14540)
SEAL no ? STATE-DETAIL LOGIN-STATE EXPIRED The CLIENT-PRINCIPAL object has expired

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Please post a 4GL snippet so I can better understand this.

Well, for example, if the CLIENT-PRINCIPAL is expired, the statement

MESASGE statement with more complex code (which, as in this case, may raise an ERROR), is buggy in FWD. Please avoid using this for now. Rewrite this test like this:

def var l as log.
l = hcp:SEAL("access-code") no-error.
message error-status:error error-status:num-messages error-status:get-message(1).
MESSAGE "SEAL" l hcp:SEAL-TIMESTAMP "STATE-DETAIL" "LOGIN-STATE" hcp:LOGIN-STATE hcp:STATE-DETAIL.

This way, you know for sure to use either recordOrShowError or recordOrThrowError.

Constantin Asofiei wrote:

MESASGE statement with more complex code (which, as in this case, may raise an ERROR), is buggy in FWD. Please avoid using this for now. Rewrite this test like this:
[...]

This way, you know for sure to use either recordOrShowError or recordOrThrowError.

I see. Thank you.

Constantin Asofiei wrote:

Igor, before your next merge, please check/fix javadoc issues related to the 3810 code. Thanks.

Constantin, do you mean errors reported by javadoc or anything else? Thanks.

Igor Skornyakov wrote:

Constantin, do you mean errors reported by javadoc ...

Yes, errors reported by ant javadoc related to the code you wrote for this task.

Constantin Asofiei wrote:

Yes, errors reported by ant javadoc related to the code you wrote for this task.

Got it. Will fix.

1. Implemented and tested conversion and runtime support for:

CLIENT-PRINCIPAL                            kw_clnt_prl
    attributes
        DOMAIN-DESCRIPTION                  kw_domain_d
        CLIENT-TTY                          kw_clnt_tty
        AUDIT-EVENT-CONTEXT                 kw_aud_ev_c
        LOGIN-HOST                          kw_log_host
        ROLES                               kw_roles
        LOGIN-STATE                         kw_log_stat
        LOGIN-EXPIRATION-TIMESTAMP          kw_log_e_ts
        SEAL-TIMESTAMP                      kw_seal_tst
        PRIMARY-PASSPHRASE                  kw_prim_p_p
        QUALIFIED-USER-ID                   kw_qual_uid
        STATE-DETAIL                        kw_stat_det

    methods
        VALIDATE-SEAL                       kw_val_seal
        LOGOUT                              kw_logout
        INITIALIZE                          kw_init_c_p
 

Committed to the branch 3810b revision 11329.

Added uast/security/domain-n-user.p test.

Branch 3810b rebased to the trunk. Started regression testing

Please summarize what features/work remains TODO (it seems like most things, if not all of them, are complete).

Remaining features:

SECURITY-POLICY system handle
    methods
        GET-CLIENT
        LOCK-REGISTRATION
        REGISTER-DOMAIN
        SET-CLIENT
        LOAD-DOMAINS
AUDIT-POLICY system handle
    methods
        ENCRYPT-AUDIT-MAC-KEY
        REFRESH-AUDIT-POLICY

AUDIT-CONTROL system handle
    attributes
        APPL-CONTEXT-ID
        EVENT-GROUP-ID
    methods
        BEGIN-EVENT-GROUP
        END-EVENT-GROUP
        SET-APPL-CONTEXT
        CLEAR-APPL-CONTEXT
        LOG-AUDIT-EVENT
Functions
        GET-DB-CLIENT
        SET-DB-CLIENT

As Constantin has mentioned (note #4) CLIENT-PRINCIPAL:(EXPORT-PRINCIPAL,IMPORT-PRINCIPAL) have to be updated to support new attributes. I've already made minor changes to reflect the change of the sealed field datatype which was made based on testing using STATE-DETAIL attribute (4GL makes difference between the principal which was never sealed and the principal which became unsealed due to e.g. LOGOUT.

I've analyzed the result of the CLIENT-PRINCIPAL:EXPORT-PRINCIPAL method.
It seems that it is a byte array started with two bytes 0x00, 0x30 followed by a list of attributes in TLV (tag length value) format. The order of attributes depends somehow on the initialization order but not 100% directly.
The tag consists of two parts - 2-byte type subtag and two-byte attribute id. The table below describes the tag for documented attributes and some "hidden" attributes. The first "hidden" attribute is presumably a 16 bytes MAC generated from the domain access code. The semantics of the remaining two is unclear.
The 00a0 type corresponds to the CHARACTER datatypes which is stored with trailing zero byte, the 0090 type corresponds to the DATETIME-TZ datatype (its binary format is not clear at this moment but it shouldn't be a big deal). The semantics of other tag types is unclear.

AUDIT-EVENT-CONTEXT           // 00a0 000a 
CLIENT-TTY                    // 00a0 0003 
CLIENT-WORKSTATION            // 00a0 0018
DB-LIST 
DOMAIN-DESCRIPTION            // 00a0 0019
DOMAIN-NAME                   // 00a0 0001 
DOMAIN-TYPE                   // 00a0 000e 
HANDLE 
INSTANTIATING-PROCEDURE 
LOGIN-EXPIRATION-TIMESTAMP    // 0090 0010
LOGIN-HOST                    // 00a0 0004 
LOGIN-STATE                   // 000c 0000 (?) - encoded as numeric value (00 - INITIAL, 0b - LOGIN, 0a - LOGOUT, 05 - EXPIRED)
PRIMARY-PASSPHRASE            // 00a0 0006
QUALIFIED-USER-ID 
ROLES                         // 00a0 000d
SEAL-TIMESTAMP                // 0090 000b
SESSION-ID                    // 00a0 000f 
STATE-DETAIL 
TYPE 
USER-ID                       // 00a0 0002
*DOMAIN-ACCESS-CODE(?)        // 00b0 0015 - 16 bytes
* (?)                         // 00d0 001c - 8 bytes (creation/initialization timestamp ?)  
* (?)                         // 00c0 0014 - value is always 4 zero bytes in my tests
* (?)                         // 00c0 0016 - value is always 4 zero bytes in my tests

This analysis also allowed me to notice that the values of most attributes are reset by the INITIALIZE method and find a number of other minor incompatibilities between FWD support of the CLIENT-PRINCIPAL anf 4GL.

I'm not sure that we have to replace the current implementation of the :EXPORT-PRINCIPAL based on Java serialization but I think that at least semantically it should be adjusted. I also believe that it makes sense to add support of two remaining CLIENT-PRINCIPAL attributes (CLIENT-WORKSTATION and DOMAIN-TYPE).

I'm not sure that we have to replace the current implementation of the :EXPORT-PRINCIPAL based on Java serialization but I think that at least semantically it should be adjusted.

No, don't use Java serialization since it won't be compatible. I understand there are some unknowns. Please implement what you can in a compatible way, and make a list of the questions that remain. Put your best guess for a placeholder in those cases.

Marian's team may be able to shed some light on this list of questions.

I also believe that it makes sense to add support of two remaining CLIENT-PRINCIPAL attributes (CLIENT-WORKSTATION and DOMAIN-TYPE).

Agreed.

Greg Shah wrote:
.> No, don't use Java serialization since it won't be compatible. I understand there are some unknowns. Please implement what you can in a compatible way, and make a list of the questions that remain. Put your best guess for a placeholder in those cases.

Marian's team may be able to shed some light on this list of questions.

OK, thank you.

In regard to data type serialization, I wonder if the 4GL uses the same approach as for raw-transfer (see #3549). I started that work and Ovidiu took it further in #3758-15 (and later notes), though I think he only implemented the record-length replacement. The core idea here was to let the specific BDT types report their serialized sizes in a way that was compatible with the 4GL. I think we never have implemented the fully compatible raw-transfer implementation (see BufferImpl.rawTransfer()). I would eventually want the BDT classes to be able to render themselves in a 4GL compatible serialization. The current raw-transfer uses Java serialization, so if we ever have to read data serialized from 4GL it will fail. Anyway, it would be good to know if the same serialization approach is used. If so, then the same BDT serialization helpers could be used here and for raw-transfer.

Greg Shah wrote:

In regard to data type serialization, I wonder if the 4GL uses the same approach as for raw-transfer (see #3549). I started that work and Ovidiu took it further in #3758-15 (and later notes), though I think he only implemented the record-length replacement. The core idea here was to let the specific BDT types report their serialized sizes in a way that was compatible with the 4GL. I think we never have implemented the fully compatible raw-transfer implementation (see BufferImpl.rawTransfer()). I would eventually want the BDT classes to be able to render themselves in a 4GL compatible serialization. The current raw-transfer uses Java serialization, so if we ever have to read data serialized from 4GL it will fail. Anyway, it would be good to know if the same serialization approach is used. If so, then the same BDT serialization helpers could be used here and for raw-transfer.

This is interesting. I think that there is a good chance that Progress uses similar (if not exactly the same) logic in both situations.

Igor Skornyakov wrote:

I've noticed a strange side effect of the CLIENT-PRINCIPAL:EXPORT-PRINCIPAL method call.
If we run the attached export-cp-bug.p program the values of the attributes look empty (while in the serialized data they present).
If we comment out the EXPORT-PRINCIPAL() invocation, the result is as expected.

I consider this as a bug and do not think that we have to reproduce such a weird behavior in FWD. Am I right?
Thank you.

From a first look, it seems like a bug. Possibly even one that Progress might fix one day.

Marian: What is your opinion? Is there any meaning or purpose for this behavior?

Greg Shah wrote:

From a first look, it seems like a bug. Possibly even one that Progress might fix one day.

Marian: What is your opinion? Is there any meaning or purpose for this behavior?

Definitively a Progress bug, adding an initialize in between make things work again. However, since this was not reported already it means no one actually used like this before... imho it doesn't make sense anyway. The usual flow here is to call initialize or set the required attributes and then seal the whole thing so it can't be tampered with. Exporting a client principal that was not sealed already albeit possible is not really something very useful imho :)

Thanks for that opinion. We will treat this as a "quirk" which is of no known value at this time.

Igor: Please create a new task "CLIENT-PRINCIPAL:EXPORT-PRINCIPAL method quirk (4GL bug?)" with the details and testcase from #3810-179. Make that "related" to this task and make me a watcher. We will avoid that "feature" until we hit a customer's 4GL code that depends upon it.

Greg Shah wrote:

Igor: Please create a new task "CLIENT-PRINCIPAL:EXPORT-PRINCIPAL method quirk (4GL bug?)" with the details and testcase from #3810-179. Make that "related" to this task and make me a watcher. We will avoid that "feature" until we hit a customer's 4GL code that depends upon it.

Done. See #4108

Only tc_job_002 consistently fails in multiple runs of the 3810b regression testing.

Added CLIENT-PRINCIPAL CLIENT-WORKSTATION attribute and AUTHENTICATION-FAILED methods conversion and runtime support. Re-worked ClientPrincipal logic to support 4GL style serialization.
Branch 3810b revision 11338.

Added testcases/uast/security/cp-export.p test.

The following questions are still open:
1. What is the exact format of the datetime-tz instances (LOGIN-EXPIRATION-TIMESTAMP and SEAL-TIMESTAMP) serialization? This should not be difficult to figure.
2. What is the exact algorithm of the domain-access-code based MAC calculation on SEAL, LOGOUT, and AUTHENTICATION-FAILED? It is always 16 bytes and doesn't depend on possibly related SECURITY-POLICY attributes.
3. What key is used for the MAC calculation on AUTHENTICATION-FAILED instead of the domain-access-code? This method is applicable to the unsealed principal only and the only possible candidate I see at this moment is the reason argument, but it doesn't look to have much sense.

Igor Skornyakov wrote:

The following questions are still open:
1. What is the exact format of the datetime-tz instances (LOGIN-EXPIRATION-TIMESTAMP and SEAL-TIMESTAMP) serialization? This should not be difficult to figure.
2. What is the exact algorithm of the domain-access-code based MAC calculation on SEAL, LOGOUT, and AUTHENTICATION-FAILED? It is always 16 bytes and doesn't depend on possibly related SECURITY-POLICY attributes.
3. What key is used for the MAC calculation on AUTHENTICATION-FAILED instead of the domain-access-code? This method is applicable to the unsealed principal only and the only possible candidate I see at this moment is the reason argument, but it doesn't look to have much sense.

Hi Igor,

not sure if you are looking for answers on our side, afraid we don't really know (nor care as a matter of fact) the internals of how the client principal is serialized as long as we can do it then it's all good :)

On the other hand, while for encrypted data you probably do need to come up with a binary compatible implementation so you can decrypt data at rest - files, database - for this client-principal specific import/export I really think you can use whatever serialization works for you. This is not something that is created and put on the shelf for later use. There won't be any 4GL serialized client-principal data that you will need to import since this is really only used for the time span of a user session, worst case they will get all sessions destroyed when they make the switch to FWD but I'm sure that this is the least disruption when that move happens :)

Marian Edu wrote:

Hi Igor,

not sure if you are looking for answers on our side, afraid we don't really know (nor care as a matter of fact) the internals of how the client principal is serialized as long as we can do it then it's all good :)

On the other hand, while for encrypted data you probably do need to come up with a binary compatible implementation so you can decrypt data at rest - files, database - for this client-principal specific import/export I really think you can use whatever serialization works for you. This is not something that is created and put on the shelf for later use. There won't be any 4GL serialized client-principal data that you will need to import since this is really only used for the time span of a user session, worst case they will get all sessions destroyed when they make the switch to FWD but I'm sure that this is the least disruption when that move happens :)

Hi Marian,
Thank you very much for your comments. Of course, you know much better about the use cases of the exported CLIENT-PRINCIPAL. It was Greg's suggestion to get rid of the existing approach (based on Java serialization) and implement EXPORT-PRINCIPAL in 4GL compatible way.
The following excerpt from the OpenEdge documentation makes me think that it makes sense indeed:

A typical use case for exporting a sealed security token is in a remote authentication service that responds to identity management requests from ABL sessions of an n-tier application. For each request, the authentication service retrieves and imports the sealed client-principal from secure storage that is associated with a given login session key and performs the requested action (such as invoking the LOGOUT( ) method to terminate the user login session). The service then exports the sealed (and changed) client-principal, replacing the previous copy in secure storage and possibly returning it to the requesting ABL session.

The ongoing analysis of the EXPORT-PRINCIPAL clarified some things which described as unclear in note 174.

The 0x00 0xc0 type means "list" and the "attribute" 0x0014 corresponds to the principal properties. They are encoded as following

00c00014<total len> 00a00014<property len><key><value> 00a00014<property len><key><value>
Where <key> and <value> are serialized as concatenated null-terminated strings, key first.

My guess is that 00c000016 corresponds to the DB-LIST() but I cannot check it at this moment.

It was Greg's suggestion to get rid of the existing approach (based on Java serialization) and implement EXPORT-PRINCIPAL in 4GL compatible way.
The following excerpt from the OpenEdge documentation makes me think that it makes sense indeed:

Although it is not a recommended approach, we have found a general rule:

If someone can rely upon a 4GL quirk, then someone will rely upon it".

For this reason, we assume that we must have full compatibility with all such features. It is only in the case where we cannot see how someone can use the feature (badly) that we may deliberately choose to be incompatible.

In this case, it seems possible for someone to use this serialized binary format to implement a long-lived authentication token instead of a short lived session token. If they do that we will need the binary compatibility. Better to implement it now than to find out later (at runtime) that it is broken.

It seems that the quirk described in #4801 is not a bug but something intentional.
In particular, after CP is exported at least once then it is impossible to add properties (SET-PROPERTY returns no). The INITIALIZE "forgets" about the export. I do not understand the idea behind this.
Fortunately, with the new version of the ClientPrincipal class, it is easy to support this in FWD.

Actually, datetime-tz is serialized as two TLVs: the first one (tag 0x0090 ) is a long number of seconds since epoch, the second one (tag 0x00d0) is a long timezone. The id of the epoch/timezone part is 0x000b/0x0017 for the SEAL-TIMESTAMP and 0x0010/0x001b for the LOGIN-EXPIRATION-TIMESTAMP

The Javadoc for datetime.getAbsoluteTimeOffset and dataetimetz.getAbsoluteTimeOffset() are incorrect. The returned value is not the the absolute time in milliseconds since Java 'Epoch'.

The converted code

PROCEDURE dump:
    DEF INPUT PARAM step AS CHAR.

    DEF VAR token AS raw NO-UNDO.
    DEF VAR hex AS CHAR NO-UNDO.

    token = hcp:EXPORT-PRINCIPAL().
    hex = HEX-ENCODE(token).
    MESSAGE step hex.
END.

Igor Skornyakov wrote:

The converted code
[...]
throws ArrayIndexOutOfBoundsException in MESSAGE if hex is longer than 512 chars. Investigating.

Are you in ChUI? This is a hard limit on the number of characters for the terminal's width. I don't think you need to look into it, but please post a stacktrace.

We should ensure that it does not generate an abend. I don't recall what the 4GL does (e.g. does it silently truncate to the max width), but we should go ahead and match it.

Constantin Asofiei wrote:

Are you in ChUI? This is a hard limit on the number of characters for the terminal's width. I don't think you need to look into it, but please post a stacktrace.

Yes, it was ChUI, but the output was redirected to a file:

Caused by: java.lang.ArrayIndexOutOfBoundsException: 512
        at com.goldencode.p2j.ui.chui.RedirectedTerminal.addChar(RedirectedTerminal.java:1137)
        at com.goldencode.p2j.ui.chui.RedirectedTerminal.append(RedirectedTerminal.java:366)
        at com.goldencode.p2j.ui.client.chui.driver.ChuiOutputManager.append(ChuiOutputManager.java:360)
        at com.goldencode.p2j.ui.client.chui.driver.ChuiOutputManager.append(ChuiOutputManager.java:96)
        at com.goldencode.p2j.ui.chui.ThinClient.sendToStream(ThinClient.java:3750)
        at com.goldencode.p2j.ui.client.gui.WindowGuiImpl.message(WindowGuiImpl.java:1261)
        at com.goldencode.p2j.ui.client.Window.message(Window.java:2027)
        at com.goldencode.p2j.ui.chui.ThinClient.message(ThinClient.java:8308)
        at com.goldencode.p2j.ui.ClientExportsMethodAccess.invoke(Unknown Source)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:156)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:757)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:412)
        at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:348)
        at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1201)
        at com.goldencode.p2j.net.Queue.transact(Queue.java:672)
        at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:271)
        at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:211)
        at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1473)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:145)

Greg Shah wrote:

We should ensure that it does not generate an abend. I don't recall what the 4GL does (e.g. does it silently truncate to the max width), but we should go ahead and match it.

Normally for display, unless specified otherwise, 4GL uses default format for different data types... for character is x(8), meaning it will only show the first 8 characters. For numbers or if you specify a format that does not fit in the display it will throw a runtime error if the data doesn't fit into that format (specified or default).

However, since here Igor used 'message' not 'display' then regardless of the ui mode (chui/gui) the complete data is shown in the 'message bar' section regardless of the format defined for the variable. The only difference is that in tty(chui) one needs to 'press spacebar to continue' if the data doesn't fit into the two lines of the message bar, for gui the message bar has a scrollbar.

Constantin Asofiei wrote:

Are you in ChUI? This is a hard limit on the number of characters for the terminal's width. I don't think you need to look into it, but please post a stacktrace.

I've reworked my test so that this issue doesn't affect it. I think that fixing the problem is not in the scope of this task.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Are you in ChUI? This is a hard limit on the number of characters for the terminal's width. I don't think you need to look into it, but please post a stacktrace.

I've reworked my test so that this issue doesn't affect it. I think that fixing the problem is not in the scope of this task.

Please open a bug for this so we don't forget it.

1. The exact algorithm of the MAC is unclear. FWD now uses HmacMD5 which is applied to a serialized CLIENT-PRINCIPAL data. It produces MAC of the same size as in 4GL, but the value is different.
2. It is unclear which key is used for the MAC calculation on AUTHENTICATION-FAILED instead of the domain-access-code. This method is not applicable to a sealed principal so the domain-access-code which is used in SEAL and VALIDATE-SEAL cannot be used. The reason argument is optional. At this moment FWD uses " " key (a single space). From the other side, the validation of the failed CP always fails so what the MAC is for in this case is unclear for me.
3. The semantics of the 00d0 001c "hidden" attribute is unclear. In my test, it depends on the way the principal was initialized (a signature of the INITIALIZE method if it was used). The current implementation is binary compatible with 4GL but I cannot guarantee the all possible values are covered by my test.
4. The serialization of the 00c0 0016 "hidden" attribute is not implemented. Presumably is the list returned by DB-LIST method, but I could not test this so far - in my test this list is always empty.
5. For all attributes except LOGIN-EXPIRATION-TIMESTAMP if a new value is assigned the corresponding TLV moves to the end of the serialized data, or is removed if the new value is UNKNOWN. The situation with LOGIN-EXPIRATION-TIMESTAMP is different. There can be at least two TLVs if there were multiple assignments, even for the UNKNOWN value. I could not understand the logic behind this for FWD uses the standard logic for LOGIN-EXPIRATION-TIMESTAMP. This should not affect deserialization if one is consistent. However, it is unclear at this moment how one can correctly deserialize CP which contains multiple LOGIN-EXPIRATION-TIMESTAMP TLVs.

Marian: if your team can figure out the answers to #3810-201, we would like to get those details. This is less of a priority than other tasks at the moment.

Igor: Is there any more to do before a final code review for 3810b?

Greg Shah wrote:

Igor: Is there any more to do before a final code review for 3810b?

Yes, I have to finish IMPORT-PRINCIPLA re-implementation (which is easy and almost done) and implement SECURITY-POLICY remaining methods as you suggested in your mail from June, 11.

Greg Shah wrote:

Marian: if your team can figure out the answers to #3810-201, we would like to get those details. This is less of a priority than other tasks at the moment.

Greg: does this means you want us to look into how the client principal is serialized? Our tests basically just try to see if the export/import works as expected, like for encrypt/decrypt we can add tests to ensure the exported data is actually binary compatible with 4GL - like we dump various exports of CP to files so then you can compare the results when doing the same from FWD. The exact format of CP export is not documented so the only option is to follow Igor path and try to 'guess' the algorithm they use for this one :(

Marian Edu wrote:

Greg Shah wrote:

Marian: if your team can figure out the answers to #3810-201, we would like to get those details. This is less of a priority than other tasks at the moment.

Greg: does this means you want us to look into how the client principal is serialized? Our tests basically just try to see if the export/import works as expected, like for encrypt/decrypt we can add tests to ensure the exported data is actually binary compatible with 4GL - like we dump various exports of CP to files so then you can compare the results when doing the same from FWD. The exact format of CP export is not documented so the only option is to follow Igor path and try to 'guess' the algorithm they use for this one :(

Yes, that is what I mean. But the other tasks have higher priority since we do have a working version here.

1. It is unclear if the current implementation is correct for principals exported by 4GL which contain multiple LOGIN-EXPIRATION-TIMESTAMP TLVs (see #3810-201 item 5). FWD doesn't produce such TLVs.
2. If LOGIN-EXPIRATION-TIMESTAMP is defined and om import the principal in the INITIAL LOGIN_STATE is found to be expired, 4GL changes the LOGIN_STATE to EXPIRED and adds SEAL-TIMESTAMP and MAC. FWD now does the same but as with AUTENTICATION-FAILED method it uses single space string as MAC key because I do not see any reasonable candidates ((see #3810-201 item 2). And of course, the problem the MAC algorithm (#3810-201 item 1) exists here.
3. As in #3810-201 item 2 IMPORT-PRINCIPAL doesn't support DB-LIST data.
4. I'm not sure which error messages 4GL can potentially generate if the source data for IMPORT-PRINCIPAL is corrupted. FWD always generates 16366 error CLIENT-PRINCIPAL:IMPORT failed because - Internal Authentication subsystem error [28] regardless of the nature of corruption.

Igor, please check this list and let me know if there is any attribute not yet added:

client-workstation
db-list
login-expiration-timestamp
login-state
qualified-user-id
seal-timestamp
state-detail

Constantin Asofiei wrote:

Igor, please check this list and let me know if there is any attribute not yet added:
[...]

Constantin,
All these methods/attributes are added except DB-LIST. I can add conversion support and stub shortly, but the semantics is not clear for me and I do not have 4GL test.
Please find the complete list of currently supported CLIENT-PRINCIPAL attributes and methods below

    AUDIT-EVENT-CONTEXT
    AUTHENTICATION-FAILED
    CLIENT-TTY
    CLIENT-WORKSTATION
    DOMAIN-DESCRIPTION
    DOMAIN-NAME
    DOMAIN-TYPE
    EXPORT-PRINCIPAL
    GET-PROPERTY
    IMPORT-PRINCIPAL
    INITIALIZE
    LIST-PROPERTY-NAMES
    LOGIN-EXPIRATION-TIMESTAMP
    LOGIN-HOST
    LOGIN-STATE
    LOGOUT
    PRIMARY-PASSPHRASE
    QUALIFIED-USER-ID
    ROLES
    SEAL
    SEAL-TIMESTAMP
    SESSION-ID
    SET-PROPERTY
    STATE-DETAIL
    USER-ID
    VALIDATE-SEAL

Igor Skornyakov wrote:

All these methods/attributes are added except DB-LIST. I can add conversion support and stub shortly.

Conversion support and stubbed runtime is OK. Please add it.

Constantin Asofiei wrote:

Conversion support and stubbed runtime is OK. Please add it.

Sure. I will add it shortly.

Igor Skornyakov wrote:

All these methods/attributes are added except DB-LIST. I can add conversion support and stub shortly.

DB-LIST is read-only, not sure if Mihai had the time to add the test for it but basically you can use SET-DB-CLIENT function or SET-CLIENT method on SECURITY-POLICY and this should set the DB-LIST. For now we only have one database 'fwd' where there is a user entry 'fwd' with same password.

Marian Edu wrote:

DB-LIST is read-only, not sure if Mihai had the time to add the test for it but basically you can use SET-DB-CLIENT function or SET-CLIENT method on SECURITY-POLICY and this should set the DB-LIST. For now we only have one database 'fwd' where there is a user entry 'fwd' with same password.

I see. Thanks a lot for the advice, Marian!

Branch 3810b was rebased to the trunk rev. 11317. Pushed up to revision 11347

Added conversion support and stub for the CLIENT-PRINCIPAL:DB-LIST attribute.

Branch 3810b revision 11348

main-regression test passed. However, ctrl-c tests hang in multiple runs since Saturday.

Branch 3810b was rebased to the trunk rev. 11318. Pushed up to revision 11349

"ctrl-c-regression" tests passed for 3810b revision 11348

Code Review Task Branch 3810b Revision 11349

1. In progress.g, please locate these tokens in alphabetical order (I know some were added by other people):

      KW_CUR_QRY;
      KW_MAX_LVL;
      KW_REST_ROW;
      KW_INIT_C_P;
      KW_PRIM_P_P;
      KW_QUAL_UID;

2. Why did you switch ClientPrincipal to Serializable instead of Externalizable. Anything that can be transferred over the network should use Externalizable for performance. In our RemoteObject implementation, we have especially found this to be needed. It seems like this may be needed for appserver support.

3. We have a common helper (handle.invalidAttribute()) for the "not a queryable attribute" warning. This is needed for ClientPrincipal.getPrimaryPassphrase(). If that location does not report error 4052, then please add a handle.invalidAttribute(String attr, handle h, int errNum) worker, move the current handle.invalidAttribute(String attr, handle h) code into the worker and leave a new handle.invalidAttribute(String attr, handle h) that calls the worker with 4052 a the 3rd parameter. This will leave existing calling locations intact but allow specialization when the error numebr is different.

4. In the 4GL, does ClientPrincipal.argumentUnknownError() really have the misspelling "paraaneter"?

Greg Shah wrote:

Code Review Task Branch 3810b Revision 11349

1. In progress.g, please locate these tokens in alphabetical order (I know some were added by other people):

Fixed.

2. Why did you switch ClientPrincipal to Serializable instead of Externalizable. Anything that can be transferred over the network should use Externalizable for performance. In our RemoteObject implementation, we have especially found this to be needed. It seems like this may be needed for appserver support.

I've used writeReplace/readResolve and Externalizable SerializedPrincipal to re-use new versions of exportPrincipal/importPrincipal. This reduces the serialization overhead even more than the initial implementation which used Externalizable directly. This approach is an "inverted" initial one where exportPrincipal/importPrincipal used Java serialization.

3. We have a common helper (handle.invalidAttribute()) for the "not a queryable attribute" warning. This is needed for ClientPrincipal.getPrimaryPassphrase(). If that location does not report error 4052, then please add a handle.invalidAttribute(String attr, handle h, int errNum) worker, move the current handle.invalidAttribute(String attr, handle h) code into the worker and leave a new handle.invalidAttribute(String attr, handle h) that calls the worker with 4052 a the 3rd parameter. This will leave existing calling locations intact but allow specialization when the error numebr is different.

Done.

4. In the 4GL, does ClientPrincipal.argumentUnknownError() really have the misspelling "paraaneter"?

It was my misprint, fixed.

I've used writeReplace/readResolve and Externalizable SerializedPrincipal to re-use new versions of exportPrincipal/importPrincipal. This reduces the serialization overhead even more than the initial implementation which used Externalizable directly. This approach is an "inverted" initial one where exportPrincipal/importPrincipal used Java serialization.

OK no problem.

Code Review Task Branch 3810b Revision 11350

1. Line 1488 of ClientPrincipal still has the typos.

2. Please add a history entry to handle.

Greg Shah wrote:

1. Line 1488 of ClientPrincipal still has the typos.

2. Please add a history entry to handle.

Fixed. Committed revision 11351.

Code Review Task Branch 3810b Revision 11351

The code is good.

Please merge 3810b to trunk.

Igor: Please make the list of all open items/todos from this task.

Branch 3810b passed testing, merged to trunk rev 11319 and archived.

Created new task branch 3810c.

Greg Shah wrote:

Igor: Please make the list of all open items/todos from this task.

Greg,
Unfortunately, I've made less than planned at the weekend (I've spent some time trying to figure the exact algorithm for the CLINT-PRINCIPAL MAC, but w/o success) so the remaining items are the same as in #3810-173 while there is progress with SECURITY-POLICY

When can you get the SECURITY-POLICY and GET/SET-DB-CLIENT() functions implemented? The audit features can be delayed while you work on other higher priority items.

Greg Shah wrote:

When can you get the SECURITY-POLICY and GET/SET-DB-CLIENT() functions implemented? The audit features can be delayed while you work on other higher priority items.

I plan to finish tomorrow. Except maybe LOAD-DOMAINS.

Igor Skornyakov wrote:

Added conversion support and stub for the CLIENT-PRINCIPAL:DB-LIST attribute.

Branch 3810b revision 11348

I complete the tests for db-list attribute by including set-db-client() for a tenant database. We create a database named fwd_tenant who clone fwd and who has multi tenancy enabled with command
proutil D:\~your_path~\goldencode\fwd_tenant\fwd_tenant.db -C enablemultitenancy.

Mihai Popescu-Tiganea wrote:

I complete the tests for db-list attribute by including set-db-client() for a tenant database. We create a database named fwd_tenant who clone fwd and who has multi tenancy enabled with command
proutil D:\~your_path~\goldencode\fwd_tenant\fwd_tenant.db -C enablemultitenancy.

Thank you very much, Mihai!

Igor Skornyakov wrote:

Mihai Popescu-Tiganea wrote:

I complete the tests for db-list attribute by including set-db-client() for a tenant database. We create a database named fwd_tenant who clone fwd and who has multi tenancy enabled with command
proutil D:\~your_path~\goldencode\fwd_tenant\fwd_tenant.db -C enablemultitenancy.

Thank you very much, Mihai!

The idea here Igor is that only 'multitenant' databases will show up in that list, it doesn't need to have any tables defined as multi-tenant... enable multitenancy will do it.

Let us know if you need any further help to figure out the serialization format of client principal export or you now have all the data.

Marian Edu wrote:

The idea here Igor is that only 'multitenant' databases will show up in that list, it doesn't need to have any tables defined as multi-tenant... enable multitenancy will do it.

Let us know if you need any further help to figure out the serialization format of client principal export or you now have all the data.

Thank you, Marian. At this moment I'm working on SECIRITY-POLICY support. I will let you if I will have more questions on this regard.
However, it will be great if you will clarify the situation with LOGIN-EXPIRATION-TIMESTAMP (see #3810-201 item 5).

We have a couple of somehow more complicated scenarios for this security related feature, to cover those the tests and especially the setup are going to be more complicated so maybe we should first determine if this is needed now or it can be left for later.

1. multi tenant databases, with actual tenants/groups/domains.
2. single sign-on.

For the first case we need to define a proper multi-tenant database, with tables partitioned by tenant, there can be regular tenants and super tenants (have access to data from all tenants). No Idea if this is supported in FWD but it's far more than the security related feature - setting the domain-name on the client principal is what we are after here. The core functionality of multi-tenancy is much more complex though. We did added another 'oetable' domain (beside the default one - name is empty string) and created a user for it just to test the domain-name/user-id but this doesn't really affect multi-tenancy. Using another domain with 'oslocal' (windows) authentication also works proven there is one user name created locally, we don't use windows domains so this is not something we can test but it should (hopefully) also work for unix.

For single sign-on this works with external authentication system (usually active directory) in conjunction with PASOE, we do not have the infrastructure to test something like that - no windows domain server :(

We can maybe test something with OEREALM but as said this can become quite complicated and maybe there is no need for this just yet.

@Greg, is that something that we should consider or too advanced for time being?

Multi-tenancy is definitely not in use for the immediate projects, but we will need it in about 3 months. It can be deferred.

In regard to single sign-on (SSO), can you explain the feature a bit more?

• Is it only in PASOE that this can be used? In other words, it is never directly used from GUI clients, batch clients or classic appserver?
• How is it activated in OE?
• How does the authentication server get configured so that the OE runtime knows how to access it?
• What authentication credentials get passed?
• What does it affect?
  • what is the practical result of successful authentication or failed authentication?
  • Does it set the current database user, like SETUSERID?
  • What changes are made in the security state as a result?
  • Does it affect something else?
• What 4GL language features are associated with SSO?

What is the right ErrorManager method to report an error, so that error-status:num-messages = 1 and error-status:get-message(1) return the message?
Thank you.

Igor Skornyakov wrote:

What is the right ErrorManager method to report an error, so that error-status:num-messages = 1 and error-status:get-message(1) return the message?
Thank you.

Please disregard, sorry.

Added runtime support for the SECURITY-POLICY methods: GET-CLIENT, REGISTER-DOMAIN, LOCK-REGISTRATION, and SET-CLIENT. The SET-CLIENT support is incomplete as only some of LOGIN-STATE values are supported: (INITIAL, EXPIRED, LOGIN, LOGOUT, FAILED). The other thing is that LOAD-DOMAINS is not yet supported as its semantics is not clear enough (including the SET-CLIENT operations for loaded domains).

The changes committed to the branch 3810c. Added testcases/uast/security/domains.p test.

Greg Shah wrote:

Multi-tenancy is definitely not in use for the immediate projects, but we will need it in about 3 months. It can be deferred.

OK, we leave that for later then. The only test we did with a multi-tenant database was to test the DB-LIST property of client principal so Igor could figure out how that gets serialized when exported.

In regard to single sign-on (SSO), can you explain the feature a bit more?

• Is it only in PASOE that this can be used? In other words, it is never directly used from GUI clients, batch clients or classic appserver?

The 4GL session has a authentication systems registry and there are 3 types of built in authentication systems - oetable (classic user table in database), oelocal (local OS authentication), extsso for external single sign-on. The first two supports authentication, the last one does not - the authentication is handled by the external sso system and this is usually used in PASOE environment with Spring security. There is also an option to define custom authentication systems that can either support authentication or just sso.

1. oetable - authenticate the user against database users, after authentication the client principal is sealed automatically
2. oelocal - authenticate the user against local OS users (tested for windows but unix should be supported as well), after authentication the client principal is sealed automatically
3. extsso - does not authenticate, so the ABL needs to trust that authentication system and then seal the client principal itself using the access code of the client principal domain

For each of those authentication system it is possible to set ABL callback procedure that allows the application code to take part of the authentication process.

• AuthenticateUser that is called after the authentication system successfully validated the user identity, the client principal is sent as input not sealed but there are limited operations allowed on it, the procedure can further decide if the authentication should succeed or fail. This is not called for SSO authentication system since those do not provide authentication.
• AfterSetIdentity that is called after the client principal was sealed and identity was set - one one database with set-db-client or all connected databases using security-policy:set-client. This is always called, even for SSO authentication systems.

Security domains are defined on each progress database and are associated with one particular authentication system, can also be enabled/disabled at runtime - the dba's can do that through data administration tool. Database users can also be attached to specific security domain - only if the domain uses the oetable authentication system.

Domain registry - used to access those security domains. This is basically one session domain registry that is used when setting or validate the identity of a user (client principal). SECURITY-POLICY system handle has methods for setting-up the registry - LOAD-DOMAINS will load the security domains defined in one database, REGISTER-DOMAIN allows one to manually define those security domains but that works only with custom defined authentication systems (not the builtin ones).

• How is it activated in OE?

There are menu entries in Data Administration -> Security that supports management of authentication systems and security domains.

• How does the authentication server get configured so that the OE runtime knows how to access it?

OE runtime does not access the SSO system, in case of PASOE the authentication is done by Spring security and the ABL will receive a sealed client principal (unless an ABL callback is set to add more information, mainly just use set-property).

• What authentication credentials get passed?

Only the client principal object is passed so it will have various properties set user id, domain name, domain type, etc.

• What does it affect?
  • what is the practical result of successful authentication or failed authentication?

For successful the ABL receives a valid client principal that is not sealed so the ABL must decide if to trust the domain and user and if so seal the client principal using the domain access code.

• Does it set the current database user, like SETUSERID?

Nope, for SSO client identity is not set unless SET-DB-CLIENT or SECURITY-POLICY:SET-CLIENT is called.

• What changes are made in the security state as a result?

Nothing changes before the client principal is set on a database or validated.

• Does it affect something else?

Not unless the domain user has a tenant set when multi-tenancy is used - in that case the whole database is 'segmented' for that particular tenant, well only tables that are partitioned by tenant.

• What 4GL language features are associated with SSO?

Nothing specific to SSO in 4GL.

Greg Shah wrote:

3. We have a common helper (handle.invalidAttribute()) for the "not a queryable attribute" warning. This is needed for ClientPrincipal.getPrimaryPassphrase(). If that location does not report error 4052, then please add a handle.invalidAttribute(String attr, handle h, int errNum) worker, move the current handle.invalidAttribute(String attr, handle h) code into the worker and leave a new handle.invalidAttribute(String attr, handle h) that calls the worker with 4052 a the 3rd parameter. This will leave existing calling locations intact but allow specialization when the error numebr is different.

There is a catch there, not specified in the PSC docs either, the attribute is write-only in all places but in the ABL callback procedure if that is set for the authentication system used by the security domain. Inside the AuthenticateUser callback procedure accessing this attribute of the client principal does not throw an error. For builtin authentication systems the value of the passphrase is not really of any good as it is already kinda obfuscated - at it will all be only '*', the number of stars is equal with the length of the original password plus one (as far as we can tell from the tests). For custom authentication systems that provides authentication (not just SSO) this is the actual cleartext password set on the client principal :(

Igor Skornyakov wrote:

The other thing is that LOAD-DOMAINS is not yet supported as its semantics is not clear enough (including the SET-CLIENT operations for loaded domains).

LOAD-DOMAINS can be called to load all security domains defined on a database in the session registry. Once the domains from one database were loaded you can not load the domains from another database - trying to do so will throw an error. One can call LOAD-DOMAINS using the same database multiple times, each time the registry will be swipe clean and reloaded - the security domains can be update at runtime so this can be used to refresh the registry. The call will fail after LOCK-REGISTRATION is called. There is no way to tell what is the content of the registry - aka, what security domains are available.

Marian Edu wrote:

There is a catch there, not specified in the PSC docs either, the attribute is write-only in all places but in the ABL callback procedure if that is set for the authentication system used by the security domain. Inside the AuthenticateUser callback procedure accessing this attribute of the client principal does not throw an error. For builtin authentication systems the value of the passphrase is not really of any good as it is already kinda obfuscated - at it will all be only '*', the number of stars is equal with the length of the original password plus one (as far as we can tell from the tests). For custom authentication systems that provides authentication (not just SSO) this is the actual cleartext password set on the client principal :(

Wow! Thanks for pointing this out! Greg, may be it makes sense to add corresponding getters to the ClienPrincipalResource/ClientPrincipal?

Marian Edu wrote:

LOAD-DOMAINS can be called to load all security domains defined on a database in the session registry. Once the domains from one database were loaded you can not load the domains from another database - trying to do so will throw an error. One can call LOAD-DOMAINS using the same database multiple times, each time the registry will be swipe clean and reloaded - the security domains can be update at runtime so this can be used to refresh the registry. The call will fail after LOCK-REGISTRATION is called. There is no way to tell what is the content of the registry - aka, what security domains are available.

Thank you very much Marian! It seems that I've deciphered a substantial part of the LOAD-DOMAINS logic and corresponding aspects of the SET-CLIENT functionality. It is pretty tricky...

@Greg, is that something that we should consider or too advanced for time being?

In regard to single sign-on, put that on hold for now. I will check with the client to see if it is needed.

Greg, may be it makes sense to add corresponding getters to the ClienPrincipalResource/ClientPrincipal?

Yes. Just make sure that there are appropriate TODO comments that explain that access is only allowed from a specific callback (which we don't support yet, right?).

Greg Shah wrote:

Yes. Just make sure that there are appropriate TODO comments that explain that access is only allowed from a specific callback (which we don't support yet, right?).

I see, thank you. Marian, can a take a looks at a test with such callback?

Igor Skornyakov wrote:

Greg Shah wrote:

Yes. Just make sure that there are appropriate TODO comments that explain that access is only allowed from a specific callback (which we don't support yet, right?).

I see, thank you. Marian, can a take a looks at a test with such callback?

Igor, the callback procedure is in security/callback (abl_auth.p). This basically just pass on the callback by invoking the super if any, in tests we define overrides for those procedures and set the test procedure as super in the session so that one gets called.

Tests for abl authentication callbacks are done in security/auth, for passphrase we have one specific test in there. There are two cases we test there, one using the built in oetable authentication (fwd user in fwd database) and one using a custom authentication system that supports authentication and we have a security domain that uses that authentication system (FWDAUTH). Both authentication systems have the abl callback procedure set (security/callback/abl_auth.p).

In first case the password is readable but full of starts :)
In second case you get the cleartext password in the callback so the authentication takes place there.

To update the authentication systems you need to use the Data Administration tool, select the fwd database and then go to Admin->Security->Domain Maintenance->Authentication Systems. There you can update the builtin ones by setting the callback procedure, use relative path: security/callback/abl_auth.p.
And add one custom one 'FWDAUTH' that have 'enable authentication' and uses the same callback.

To create the FWDAUTH domain you need to go to Admin->Security->Domain Maintenance->Domains and make sure you select the FWDAUTH authentication system for this domain.

Otherwise those can be imported, Mihai will push the data dump for authentication system and domains in db folder so you can afterwards load those from there - Admin->Load Data and Definitions->Security Domains.

Greg Shah wrote:

Greg, may be it makes sense to add corresponding getters to the ClienPrincipalResource/ClientPrincipal?

Yes. Just make sure that there are appropriate TODO comments that explain that access is only allowed from a specific callback (which we don't support yet, right?).

Just to make it clear, the callbacks have nothing to do with SSO, those can be set for any authentication system including the builtin ones supported by PSC. What you should maybe check with the client that needs that is what authentication system they use and if there are any ABL callbacks set.

Marian Edu wrote:

Just to make it clear, the callbacks have nothing to do with SSO, those can be set for any authentication system including the builtin ones supported by PSC. What you should maybe check with the client that needs that is what authentication system they use and if there are any ABL callbacks set.

Got it. Thanks a lot!

Does anybody know how the OpenEdge crypto [https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dmadm%2Fspecifying-encrypted-passwords.html%23wwID0EMR1Q] algorithm works?
Thank you.

I've made a small change in the ConnectionManager.getPersistence() method. It now returns null instead of throwing NPE if getDatabase() returns null.
Is it OK?
Thank you.

Igor Skornyakov wrote:

Does anybody know how the OpenEdge crypto [https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dmadm%2Fspecifying-encrypted-passwords.html%23wwID0EMR1Q] algorithm works?
Thank you.

I don't know the answer.

One quick thing to check is if "oec" (OpenEdge crypto) is the same as ENCODE. Compare the results of genpassword with the equivalent results of ENCODE to check this.

I implemented ENCODE support in #27.

Greg Shah wrote:

I don't know the answer.

One quick thing to check is if "oec" (OpenEdge crypto) is the same as ENCODE. Compare the results of genpassword with the equivalent results of ENCODE to check this.

Thank you for a hint! I will check.

Igor Skornyakov wrote:

I've made a small change in the ConnectionManager.getPersistence() method. It now returns null instead of throwing NPE if getDatabase() returns null.
Is it OK?

I think it's ok, though I looked at most of the callers and none of them catch NPE; they all seem to expect a non-null return. Is there a legitimate use case for this API if getDatabase() returns null? Please update the javadoc accordingly.

Greg Shah wrote:

One quick thing to check is if "oec" (OpenEdge crypto) is the same as ENCODE. Compare the results of genpassword with the equivalent results of ENCODE to check this.

Not exactly :(
https://knowledgebase.progress.com/articles/Article/Genpassword-password-value-does-not-match-encode-password-value

Looks like they use the 'infamous' ENCRYPT-AUDIT-MAC-KEY - https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dvpin/password-encryption.html

Eric Faulhaber wrote:

I think it's ok, though I looked at most of the callers and none of them catch NPE; they all seem to expect a non-null return. Is there a legitimate use case for this API if getDatabase() returns null? Please update the javadoc accordingly.

Thank you. I've used getPersistence in the LOAD-DOMAINS(db-name) method. In this case, I can easily imagine that getDatabase() returns null. I will update javadoc.

Marian Edu wrote:

Not exactly :(
https://knowledgebase.progress.com/articles/Article/Genpassword-password-value-does-not-match-encode-password-value

Looks like they use the 'infamous' ENCRYPT-AUDIT-MAC-KEY - https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dvpin/password-encryption.html

Thank you very much, Marian!

It seems that we have some problems with LOAD-DOMAINS(db-num) as the connected databases ordering in FWD may be different from 4GL. This happens in particular because albeit ConnectionManager.connection is a LinkedHashMap the register method populates it from DatabaseManager.permanentConnections which is a HashSet.

Marian Edu wrote:

Looks like they use the 'infamous' ENCRYPT-AUDIT-MAC-KEY - https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dvpin/password-encryption.html

This is indeed the fact: genpassword -password PROGRESS produces string consisting of 16 zeros. Thanks again!

• load files “_sec-authentication-domain.d” and “_sec-authentication-system.d” from “db” directory in Data Administration

• _oslocal – with or without domain name
• _osusertable – with or without domain name

• test client-principal forbidden attribute changes, methods calls and other actions
• test client-principal permitted attribute changes, methods calls and other actions

• different parameters has been set to catch errors and boundaries
• quirk behavior has been tested in dedicated directory ( security\security_policy\quirks )

• for errors and boundaries
• using correct parameters to create run time logins for users but always using domains already registered in data base.

Mihai Popescu-Tiganea wrote:

As a part of security-policy we add tests regarding authorization behavior.
To run tests following actions must be made:

• load files “_sec-authentication-domain.d” and “_sec-authentication-system.d” from “db” directory in Data Administration

We have made authorization for client-principal using set-db-client in domain types:

• _oslocal – with or without domain name
• _osusertable – with or without domain name

We have explored callback procedure mounted on database for user-defined authentication system and for SSO operations:

• test client-principal forbidden attribute changes, methods calls and other actions
• test client-principal permitted attribute changes, methods calls and other actions

Method load-domains() from Security-Policy has been explored:

• different parameters has been set to catch errors and boundaries
• quirk behavior has been tested in dedicated directory ( security\security_policy\quirks )

Tests for register-domain() has been made:

• for errors and boundaries
• using correct parameters to create run time logins for users but always using domains already registered in data base.

Thank you, Mihai. I will take a look a little later.

1. LOAD-DOMAINS considers only the _User table. The functionality described in #3810-260 was not addressed.
2. SET-CLIENT tries to seal the unsealed principal. It is unclear which domain-access-code is used by 4GL in this situation. FWD currently uses a string consisting of one space (" ") as in some other cases before (see #3810-201),
3. The SET-CLIENT logic looks extremely complicated. I've tried to address all corner cases I could imagine but I could overlook some.

Committed to 3810c revision 11326.

The final version of the test us testcases/uas/security/domains.p revision 1877.

Igor Skornyakov wrote:

Finished implementation and testing of the SECURITY-POLICY:LOAD-DOMAINS runtime support and updated SECURITY-POLICY:SET-CLIENT runtime support.
The following issues are still open

1. LOAD-DOMAINS considers only the _User table. The functionality described in #3810-260 was not addressed.

LOAD-DOMAINS doesn't use the _User table per se, it just load the domain registry into the session (security-policy) so tht will be used when you seal a client principal of when you use SET-CLIENT. Only if the authentication system used by the security domain is _oetable then the _User table is used.

1. SET-CLIENT tries to seal the unsealed principal. It is unclear which domain-access-code is used by 4GL in this situation. FWD currently uses a string consisting of one space (" ") as in some other cases before (see #3810-201),

If the client principal is not already sealed then SET-CLIENT will use the authentication system of the respective user security domain to authenticate the user, if that works it seals the client using the security domain access code - the one from security registry. The registry is loaded using LOAD-DOMAIN or manually setup using REGISTER-DOMAIN, so there is no default access code but the one defined for the domain.

1. The SET-CLIENT logic looks extremely complicated. I've tried to address all corner cases I could imagine but I could overlook some.

1. if client principal is not sealed then it tries to authenticate using the session domain registry. It fails if domain not found or authentication error, else seals the CP using the domain access code.
2. if/when sealed it validates the seal, fails if sealed with an invalid domain or domain access code doesn't match
3. for each connected database it tries to set the identity on the database using the CP. The trick here is this will behave like when SET-DB-CLIENT is used, and that one uses the database's own security domain registry - if the domain security that sealed the CP is not found in database's registry then it does not change the identity on that respective database although it still return true.

Thank you very much, Marian!

LOAD-DOMAINS doesn't use the _User table per se, it just load the domain registry into the session (security-policy) so tht will be used when you seal a client principal of when you use SET-CLIENT. Only if the authentication system used by the security domain is _oetable then the _User table is used.

My test (uast/security/domains.p) uses LOAD-PRINCIPAL with the database which contains only _User table, and it demonstrates highly non-trivial logic. I understand from your comment that it is just the top of the iceberg. Can you please provide a reference for a more detailed description of the whole thing?
Thank you.

If the client principal is not already sealed then SET-CLIENT will use the authentication system of the respective user security domain to authenticate the user, if that works it seals the client using the security domain access code - the one from security registry. The registry is loaded using LOAD-DOMAIN or manually setup using REGISTER-DOMAIN, so there is no default access code but the one defined for the domain.

As my test demonstrates, it is impossible to SET-CLIENT with REGISTER-DOMAIN. It seems that one can use either REGISTER-DOMAIN or LOAD-DOMAINS. Is this correct? As I wrote at this moment, the only _User table-based logic is implemented and corresponding domains do not have a domain access code. The validation is performed by comparing the user password and principal PRIMARY-PASSPHRASE. At least how it looks in my test.

It might look complicated but what it actually does is:

1. if client principal is not sealed then it tries to authenticate using the session domain registry. It fails if domain not found or authentication error, else seals the CP using the domain access code.
2. if/when sealed it validates the seal, fails if sealed with an invalid domain or domain access code doesn't match
3. for each connected database it tries to set the identity on the database using the CP. The trick here is this will behave like when SET-DB-CLIENT is used, and that one uses the database's own security domain registry - if the domain security that sealed the CP is not found in database's registry then it does not change the identity on that respective database although it still return true.

The devil (as usual) hides in details. I mean that it appeared tricky to return the same error messages (and leave the principal in the same state) as 4GL does, especially when the principal is not sealed and the SEAL operation fails (e.g. due to expiration or missed mandatory fields)

Greg. It seems that what I've implemented regarding LOAD-DOMAINS is just a small portion of the functionality. Should I continue to work on it or it is OK to leave it as-is for now?
Thank you.

Unfortunately, I see a lot of incompatibilities between the current implementation of SET-DB-CLIENT in FWD and 4GL. See testcases/uast/security/set-db-client.p. I think that it will take at least 1 day to fix it.

All main-regression tests passed in two runs for 3810c

All ctrl-c tests passed for 3810c

1. SET-DB-CLIENT functionality is similar to the SET-CLIENT one in the sense that it uses only the _User table and has the same issue with the domain-access-code (see #3810-262).
2. SET-DB-CLIENT(dbnum) may be incompatible with 4GL because of different databases' ordering (see #3810-258). The same is applicable to the SET-CLIENT(dbnum).

Committed to 3810c revision 11327.

Apart from the mentioned functionality issues, the branch is ready for the code review.

Igor Skornyakov wrote:

1. SET-DB-CLIENT(dbnum) may be incompatible with 4GL because of different databases' ordering (see #3810-258). The same is applicable to the SET-CLIENT(dbnum).

Igor, ordering isn't probably relevant but the behavior should be equivalent to using LDBNAME function, aka those two statements should be equivalent:

SET-DB-CLIENT ( 1 ).
SET-DB-CLIENT ( LDBNAME ( 1 ) ).

Marian Edu wrote:

Igor, ordering isn't probably relevant but the behavior should be equivalent to using LDBNAME function, aka those two statements should be equivalent:
[...]

Thank you, Marian,
The two statements you've mentioned are equivalent with FWD.

After multiple runs of the regression test, only gso_ctrlc_3way_tests failed.

Code Review Task Branch 3810c Revision 11327

I don't have any functional concerns here. There are some formatting/code std things:

1. methods_attributes.rules needs a history entry.

2. SecurityOps.validateCP(), SecurityPolicyManager.addDomainUser(), SecurityPolicyManager.PersistentDomain (and its methods) need javadoc.

3. SecurityPolicyManager.PersistentDomain needs extends to be split in a separate line.

4, Please fix the indents of ClientPrincipal.sealFailure() to be 3 characters per indent level. This is also a problem for SecurityPolicyManager.loadDomainsFailed() and the methods that follow it.

5. Please ensure that each member variable and method is separated by 1 blank line (sometimes you put 0 blank lines and sometimes you put 2 blank lines). You can see both of these problems in multiple places in your update. The code at the bottom of SecurityPolicyManager shows them clearly.

Eric/Ovidiu: Please review the persistence related changes.

Greg Shah wrote:

Code Review Task Branch 3810c Revision 11327

I don't have any functional concerns here. There are some formatting/code std things:

1. methods_attributes.rules needs a history entry.

2. SecurityOps.validateCP(), SecurityPolicyManager.addDomainUser(), SecurityPolicyManager.PersistentDomain (and its methods) need javadoc.

3. SecurityPolicyManager.PersistentDomain needs extends to be split in a separate line.

4, Please fix the indents of ClientPrincipal.sealFailure() to be 3 characters per indent level. This is also a problem for SecurityPolicyManager.loadDomainsFailed() and the methods that follow it.

5. Please ensure that each member variable and method is separated by 1 blank line (sometimes you put 0 blank lines and sometimes you put 2 blank lines). You can see both of these problems in multiple places in your update. The code at the bottom of SecurityPolicyManager shows them clearly.

Fixed in revision 11328.

All ctrl-c tests passed for 3810c

In rev 11329 I've fixed the ClientPrincipal.sealFailure() indents and the extra lines. I also fixed a missing newline in a { else. Please look at the revision carefully to get the formatting right.

The methods in SecurityPolicyManager.PersistentDomain still need javadoc.

Igor: Please rebase 3810c.

If the persistence changes are OK, you'll merge to trunk.

Eric/Ovidiu: When can you do the review?

Greg Shah wrote:

In rev 11329 I've fixed the ClientPrincipal.sealFailure() indents and the extra lines. I also fixed a missing newline in a { else. Please look at the revision carefully to get the formatting right.

Sorry, I have looked at the code but it seems that I'm unable to see such issues. I'm working in the background on Checkstyle configuration to validate formatting automatically.

The methods in SecurityPolicyManager.PersistentDomain still need javadoc.

Fixed.

Greg Shah wrote:

Igor: Please rebase 3810c.

If the persistence changes are OK, you'll merge to trunk.

Eric/Ovidiu: When can you do the review?

Eric/Ovidiu: I understand that Greg has ConnectionManager.readUserTable() in mind. I've also used some of the ConnectionManager methods (such as getgetPersistence, getConnectedDbCount and pdbName in SecurityPoliceManager and SecirityOps.

I've tried to rebase 3810c to the trunk. The rebase operation was successful, but bzr push failed with "bzr: ERROR: These branches have diverged". How can I deal with that? The information I've found on the Internet is confusing.
Thank you.

Igor Skornyakov wrote:

I've tried to rebase 3810c to the trunk. The rebase operation was successful, but bzr push failed with "bzr: ERROR: These branches have diverged". How can I deal with that? The information I've found on the Internet is confusing.

Are you still bound to the branch (did you use "bzr unbind" before rebasing)?

Constantin Asofiei wrote:

Are you still bound to the branch (did you use "bzr unbind" before rebasing)?

Yes. I did "bzr unbind" before rebasing and the branch is still unbound.

What does bzr status and bzr info --v show? Did you use other commands beside bzr rebase-continue and bzr resolve, after the bzr rebase command?

Constantin Asofiei wrote:

What does bzr status and bzr info --v show? Did you use other commands beside bzr rebase-continue and bzr resolve, after the bzr rebase command?

There was no conflict so I had no need to use even bzr rebase-continue and bzr resolve
The bzr info -v output is:

Standalone tree (format: 2a)
Location:
  branch root: .

Related branches:
  push branch: /home/ias/secure/code/p2j_repo/p2j/active/3810c

Format:
       control: Meta directory format 1
  working tree: Working tree format 6
        branch: Branch format 7
    repository: Repository format 2a - rich roots, group compression and chk inventories

Control directory:
         1 branches

In the working tree:
      3511 unchanged
         0 modified
         0 added
         0 removed
         0 renamed
         1 unknown
        61 ignored
       189 versioned subdirectories

Branch history:
     11331 revisions
      5319 days old
   first revision: Mon 2004-12-06 09:55:52 +0000
  latest revision: Sun 2019-06-30 16:34:57 +0300

Repository:
     18582 revisions

• checkout trunk and 3810c
• rebase 3810c (latest rev is 11330 on trunk 11319)
• do a bzr push --overwrite /opt/secure/code/p2j_repo/p2j/active/3810c instead of your ~/secure folder.

I assume bzr status before rebase showed no changes in your local 3810c, right?

Constantin Asofiei wrote:

The rebase most likely failed for some reason; you can try again (on devsrv01, to be faster):

• checkout trunk and 3810c
• rebase 3810c (latest rev is 11330 on trunk 11319)
• do a bzr push --overwrite /opt/secure/code/p2j_repo/p2j/active/3810c instead of your ~/secure folder.

Sorry, I forgot to use --overwrite key. With it, the push passed OK. Sorry for disturbing and thanks a lot for your help.

The branch 3810c rebased to the trunk rev. 11320. The new revision is 11331.

Igor Skornyakov wrote:

Greg Shah wrote:

Igor: Please rebase 3810c.

If the persistence changes are OK, you'll merge to trunk.

Eric/Ovidiu: When can you do the review?

Eric/Ovidiu: I understand that Greg has ConnectionManager.readUserTable() in mind. I've also used some of the ConnectionManager methods (such as getgetPersistence, getConnectedDbCount and pdbName in SecurityPoliceManager and SecirityOps.

• TAB-alignment after @param and @return in javadoc and description on next line. The categories separated by an empty line;
• and the space after while keyword (the catch are OK);
• and lack of periods (.) at the end of javadoc phrases.

BTW: in r11331 SecurityOps lacks a H-entry and there are a lot of unused import statements, peaking with a strange import sun.net.www.content.audio.*; declaration.

Thank you Ovidiu.

I reviewed the db-related code. The ConnectionManager.readUserTable() looks fine to me. I do not see any issue from using the other methods you mentioned.
There are a few minor formatting issues, like:

• TAB-alignment after @param and @return in javadoc and description on next line. The categories separated by an empty line;
• and the space after while keyword (the catch are OK);
• and lack of periods (.) at the end of javadoc phrases.

I've fixed a number of formatting issues (including ones in javadoc) reported by Checkstyle. Some may still exist. Is it critical?

BTW: in r11331 SecurityOps lacks a H-entry and there are a lot of unused import statements, peaking with a strange import sun.net.www.content.audio.*; declaration.

Fixed.

Committed to 3810c revision 11332.

Can 3810c be merged into the trunk?
Thank you.

Can 3810c be merged into the trunk?

Yes, go ahead.

Branch 3810c was merged to trunk rev 11321 and archived.

Marian,
The current version of SET-DB-CLIENT fails when the database has an empty _User table. My 4GL test fails in this situation as well, however, the original 4GL application works. How can you explain this and what you can suggest?
Thank you,
Igor.

Igor Skornyakov wrote:

Marian,
The current version of SET-DB-CLIENT fails when the database has an empty _User table. My 4GL test fails in this situation as well, however, the original 4GL application works. How can you explain this and what you can suggest?
Thank you,
Igor.

Hi Igor, normally you should be able to set the user identity even if the database does not have records in _user table but it depends on what the database have setup as authentication systems and security domains. Are you trying that on a database where you have the data we've exported for security domains loaded?

Igor, I think something is missing in your implementation. FWD has the legacy security domains and domain types managed via SecurityOps.dbDomains and SecurityOps.dbDomainTypes - these are not used by your changes.

There is a previously written SecurityOps.validateClient, which is now made obsolete by your changes, but SecurityOps.validateCP doesn't use the logic it has there.

Constantin Asofiei wrote:

Igor, I think something is missing in your implementation. FWD has the legacy security domains and domain types managed via SecurityOps.dbDomains and SecurityOps.dbDomainTypes - these are not used by your changes.

There is a previously written SecurityOps.validateClient, which is now made obsolete by your changes, but SecurityOps.validateCP doesn't use the logic it has there.

Thank you, Constantin. I've got an impression that the SecurityOps.validateClient is mostly a stub. At least it failed to validate a correct principal in my test. I will check again.

Marian Edu wrote:

Hi Igor, normally you should be able to set the user identity even if the database does not have records in _user table but it depends on what the database have setup as authentication systems and security domains. Are you trying that on a database where you have the data we've exported for security domains loaded?

Thank you, Marian. No, I had to switch to another task and had no time to test that.

• if the associated secority domain type is _oeusertable, perform auth against the _user table
• otherwise, do what validateClient does.

Constantin Asofiei wrote:

Igor, my understanding is that only _oeusertable security domain type authenticates against the _user table - so please change SecurityOps.validateCP to check CLIENT-PRINCIPAL:DOMAIN-TYPE and/or DOMAIN-NAME:

• if the associated secority domain type is _oeusertable, perform auth against the _user table
• otherwise, do what validateClient does.

OK. How can I set the security domain type in a test?
Thank you.

Greg,
Should I create a new 3810 branch for the change mentioned by Constantin?
Thank you.

Please use 3855b.

Greg Shah wrote:

Please use 3855b.

OK, thanks

It seems that there are a number of TODO items in SecurityOps which can be resolved now that we have the changes from this task in the trunk.

Igor: Please review all TODOs in SecurityOps and see which of those can now be resolved. If there are other answers needed before that implementation can be done, please post questions here for Marian's team. If any of this cannot be done quickly because it is larger in scope, document the items here.

Greg Shah wrote:

It seems that there are a number of TODO items in SecurityOps which can be resolved now that we have the changes from this task in the trunk.

Igor: Please review all TODOs in SecurityOps and see which of those can now be resolved. If there are other answers needed before that implementation can be done, please post questions here for Marian's team. If any of this cannot be done quickly because it is larger in scope, document the items here.

Got it. Thanks

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Igor, my understanding is that only _oeusertable security domain type authenticates against the _user table - so please change SecurityOps.validateCP to check CLIENT-PRINCIPAL:DOMAIN-TYPE and/or DOMAIN-NAME:

• if the associated secority domain type is _oeusertable, perform auth against the _user table
• otherwise, do what validateClient does.

How can I find information about the domain configuration in the situation where my changes cause failure?
Thank you.

Igor Skornyakov wrote:

Greg Shah wrote:

Please use 3855b.

OK, thanks

No, please use 3888b - Eugenie needs it before me.

Igor Skornyakov wrote:

OK. How can I set the security domain type in a test?

Although this can be set (if not sealed) it's just informative, the domain name is used against the security registry when the CP is authenticated and then this is set to the type set for that domain in the registry. Bref, what it really counts there is the domain name - SET-DB-CLIENT will use the database security registry to find the domain and the authentication system (type) to use.

Marian Edu wrote:

Although this can be set (if not sealed) it's just informative, the domain name is used against the security registry when the CP is authenticated and then this is set to the type set for that domain in the registry. Bref, what it really counts there is the domain name - SET-DB-CLIENT will use the database security registry to find the domain and the authentication system (type) to use.

Thank you, Marian. Now I see that my understanding of the domain security was incomplete (and partially incorrect) and I have to rework my implementation. Fortunately, it seems to be not a big deal

Constantin Asofiei wrote:

Igor, my understanding is that only _oeusertable security domain type authenticates against the _user table - so please change SecurityOps.validateCP to check CLIENT-PRINCIPAL:DOMAIN-TYPE and/or DOMAIN-NAME:

• if the associated secority domain type is _oeusertable, perform auth against the _user table
• otherwise, do what validateClient does.

Constantin. I understand that validateClient essentially validates the domain-access-code of the client-principal against the one taken from the FWD directory. It seems to be OK if there is only one connected database and the FWD directory is in sync with its _sec-authentication-domain/_sec-authentication-system tables. For multiple databases, this is not correct logic. In any case, I have to test.

Greg. I understand that at this moment I do not have to support _extsso and _oslocal system types. Is that correct?

Greg. I understand that at this moment I do not have to support _extsso and _oslocal system types. Is that correct?

At this time we don't need _extsso.

I'm not sure about _oslocal. We may need that one or will need to very soon. What is the implication of changes for _oslocal?

Greg Shah wrote:

I'm not sure about _oslocal. We may need that one or will need to very soon. What is the implication of changes for _oslocal?

Well, this should not be very complicated but will require some native (and platform-dependent) code as it delegates the user authentication to the operating system.
Please note also that at this moment we do not support the authentication callbacks.

We actually have much of that code in our spawner today (for Linux/UNIX and Windows). But it will take some time to refactor and implement. For now, please put _oslocal on hold.

Please create a new note that has the full list of work on this task that is not complete. When we come back to this work I want it to be simple to understand what needs to happen.

Greg Shah wrote:

We actually have much of that code in our spawner today (for Linux/UNIX and Windows). But it will take some time to refactor and implement. For now, please put _oslocal on hold.

Please create a new note that has the full list of work on this task that is not complete. When we come back to this work I want it to be simple to understand what needs to happen.

OK.

I've changed support for SET-DB-CLIENT for non-default domains. Now, in this case, the validation is based on the FWD directory, as before, but a sealing was added for the unsealed principal. This should fix the issue described in #4121-54.
However, some more changes are required as it appears that my understanding of 4GL domains security was incomplete.
I hope to fix it tomorrow.

Committed to the branch 3888b revision 11328

For some reasons 4GL includes a UNIQUE index on _domain-id column on _sec-authentication-domain table data definition dump. Actually, the values in this column are not unique. I had to manually update _sec-authentication-domain.df to be able to import data.

Igor Skornyakov wrote:

What I'm doing wrong?

What are you trying to do there? There is no need to load meta-schema tables, every Progress databases have those already... exporting/importing data is different from 'schema' if this is what you are trying to do - there should be some .d files with exported records not .df.

If the meta-schema table is defined with specific indexes you can't change that, if you try to load security domains data and it gives you error is because you already imported that before.

Marian Edu wrote:

If the meta-schema table is defined with specific indexes you can't change that, if you try to load security domains data and it gives you error is because you already imported that before.

Thank you, Marian. I understand that _User table should not be in the .df file.

Igor Skornyakov wrote:

For some reasons 4GL includes a UNIQUE index on _domain-id column on _sec-authentication-domain table data definition dump. Actually, the values in this column are not unique. I had to manually update _sec-authentication-domain.df to be able to import data.

We have seen this behavior before, but in user data. We aren't sure how it happens but the theory is that there is existing data in the table that was inserted before a unique index is added on the table. When the unique index is added, perhaps the OE database does not check existing values so you end up with something that should never happen.

When we try to import it in FWD, our import process fails. Editing the data is a solution if you can determine which record(s) should not be there.

Marian,
If I have a domain dom in the _sec-authentication-domain table which has type _oeusertable which USERID in the _User table corresponds to 'uid@dom'? I expected that it is 'uid' but it seems to be incorrect.
Thank you.

Greg Shah wrote:

We have seen this behavior before, but in user data. We aren't sure how it happens but the theory is that there is existing data in the table that was inserted before a unique index is added on the table. When the unique index is added, perhaps the OE database does not check existing values so you end up with something that should never happen.

When we try to import it in FWD, our import process fails. Editing the data is a solution if you can determine which record(s) should not be there.

In my case, I've just removed the UNIQUE clause from the index definition in the.df file. Removing records is not an option as in this case as many default domains will not be included.

We have two problems with _User table
1. The META_USER doesn't contain DOMAIN_NAME column, so on import, the information needed for correct implementation of the validation of the users from a non-default domain is lost.
2. 4GL allows duplicated user ids with different DOMAIN_NAME while FWD does not.

Igor Skornyakov wrote:

We have two problems with _User table
1. The META_USER doesn't contain DOMAIN_NAME column, so on import, the information needed for correct implementation of the validation of the users from a non-default domain is lost.
2. 4GL allows duplicated user ids with different DOMAIN_NAME while FWD does not.

On what are you basing these assertions? Here is the converted DDL (PostgreSQL) for the meta_user table in the testcases project:

    create table meta_user (
        id int8 not null,
        userid text not null,
        password text not null,
        user_name text,
        user_misc text,
        user_number int4,
        group_number int4,
        given_name text,
        middle_initial text,
        surname text,
        telephone text,
        email text,
        description text,
        disabled bool,
        create_date_timestamp timestamp,
        create_date_offset int4,
        account_expires_timestamp timestamp,
        account_expires_offset int4,
        pwd_expires_timestamp timestamp,
        pwd_expires_offset int4,
        pwd_duration int4,
        last_login_timestamp timestamp,
        last_login_offset int4,
        logins int4,
        max_logins int4,
        max_tries int4,
        login_failures int4,
        spare1 int4,
        spare2 int4,
        spare3 text,
        spare4 text,
        tenantid int4 not null,
        domain_name text not null,
        sql_only_user bool not null,
        primary key (id)
    );

Note domain_name is there. Here is the index DDL for the same table:

drop index if exists idx__meta_user_userid;
drop index if exists idx__meta_user_user_domain_name;
drop index if exists idx__meta_user_user_sql_only_user;
create unique index idx__meta_user_userid on meta_user (upper(rtrim(userid)), upper(rtrim(domain_name)));
create index idx__meta_user_user_domain_name on meta_user (upper(rtrim(domain_name)), sql_only_user, upper(rtrim(userid)), id);
create index idx__meta_user_user_sql_only_user on meta_user (sql_only_user, id);

There is a unique index on the combination of the userid and domain_name columns (both uppercased and trimmed).

So the column should both exist and allow unique (non-null) combinations of upper(rtrim(userid)) and upper(rtrim(domain_name)).

Eric Faulhaber wrote:

Igor Skornyakov wrote:

We have two problems with _User table
1. The META_USER doesn't contain DOMAIN_NAME column, so on import, the information needed for correct implementation of the validation of the users from a non-default domain is lost.
2. 4GL allows duplicated user ids with different DOMAIN_NAME while FWD does not.

On what are you basing these assertions? Here is the converted DDL (PostgreSQL) for the meta_user table in the testcases project:

[...]

Note domain_name is there. Here is the index DDL for the same table:

[...]

There is a unique index on the combination of the userid and domain_name columns (both uppercased and trimmed).

So the column should both exist and allow unique (non-null) combinations of upper(rtrim(userid)) and upper(rtrim(domain_name)).

Interesting. And this is how the DDL looks when I convert the test program with the database in testcases/uast:

    create table meta_user (
        id int8 not null,
        userid text not null,
        password text not null,
        user_name text,
        user_misc text,
        primary key (id)
    );

Constantin Asofiei wrote:

The trunk rev of testcases project has an older version of standard.df (which has the meta definitions); try using this one.

I see. Thanks a lot!

Igor Skornyakov wrote:

Well, with new standard.df the META_USER table looks OK, but on import, the value of the DOMAIN-NAME from _User.d (attached) column is inserted into the EMAIL column.

That means the _User.d export you have is from a different meta schema.

Marian, please export the meta tables from your machine and commit it to the db/ folder in the testcases project on xfer.

Constantin Asofiei wrote:

That means the _User.d export you have is from a different meta schema.

I used 11.6 and created the database from scratch.

I've finished re-working LOAD-DOMAINS and SET-CLIENT for _oeusertable domains.
Committed to 3888b, revision 11332.
The branch 3888b is ready for the code review.

Please note that I had to manually populate META_USER table (see #3810-328) to be able to run the tests.

All regression tests passed for 3888b.

Code Review Task Branch 3888b Revision 11334

I good with the changes. I did check in rev 11335 with a minor formatting fix.

My inclination is to merge this to trunk now.

Igor/Eugenie/Constantin: Any objections?

Greg Shah wrote:

Code Review Task Branch 3888b Revision 11334

I good with the changes. I did check in rev 11335 with a minor formatting fix.

My inclination is to merge this to trunk now.

Igor/Eugenie/Constantin: Any objections?

I'm OK. The branch looks sage from regression perspective.

Greg Shah wrote:

Igor/Eugenie/Constantin: Any objections?

Greg, please note that there can be problems with non-default domains of _oeusertable type which are not configured in FWD directory (see #3810-328)

Igor Skornyakov wrote:

Greg Shah wrote:

Igor/Eugenie/Constantin: Any objections?

Greg, please note that there can be problems with non-default domains of _oeusertable type which are not configured in FWD directory (see #3810-328)

Do we need any protection code and does the feature fail the same way in the 4GL? In other words:

• The feature won't abend.
• The feature would work the same way as in the 4GL if there was no configuration.

Greg Shah wrote:

Do we need any protection code and does the feature fail the same way in the 4GL? In other words:

• The feature won't abend.
• The feature would work the same way as in the 4GL if there was no configuration.

Greg,
The problem is that import of _User.d into META_USER doesn't work correctly (the value of the DOMIAN_NAME field is inserted into the wrong column, at least in my test). This may result in the principal validation failure. I do not see how to fix this programmatically, but it can be easily fixed my manual modification of the META_USER table after import.

Greg Shah wrote:

Code Review Task Branch 3888b Revision 11334

I good with the changes. I did check in rev 11335 with a minor formatting fix.

My inclination is to merge this to trunk now.

Igor/Eugenie/Constantin: Any objections?

Greg, are you planning to do merge soon for 3888b? Or this is up to me to do?

Eugenie: Please go ahead and merge 3888b to trunk.

Greg Shah wrote:

Eugenie: Please go ahead and merge 3888b to trunk.

OK. Starting to merge.

Igor Skornyakov wrote:

Greg Shah wrote:

Do we need any protection code and does the feature fail the same way in the 4GL? In other words:

• The feature won't abend.
• The feature would work the same way as in the 4GL if there was no configuration.

Greg,
The problem is that import of _User.d into META_USER doesn't work correctly (the value of the DOMIAN_NAME field is inserted into the wrong column, at least in my test). This may result in the principal validation failure. I do not see how to fix this programmatically, but it can be easily fixed my manual modification of the META_USER table after import.

Is this caused by differences between the version of the metadata schema used in conversion as compared to the metadata schema used to export the _User.d file?

Greg Shah wrote:

Is this caused by differences between the version of the metadata schema used in conversion as compared to the metadata schema used to export the _User.d file?

I think so. I've tried to modify standard.df, but w/o success. See #3810-328

OK, so we need the matching metadata schema to be checked in to the testcases project as per the request in #3810-328.

This is not a bug, but I do wonder if we need better error checking on our import processing. For example, should we detect these cases:

• more fields in the .d than in the schema
• fewer fields in the .d than in the schema

Constantin Asofiei wrote:

Marian, please export the meta tables from your machine and commit it to the db/ folder in the testcases project on xfer.

OK, meta.df added in db/ folder... no idea how this meta-data structure is licensed if anything so don't assume this is free of any licensing constraints, hope you know what you are doing :)

this is for for version 11.7.4, there are changes from one version to another from time to time... don't have a 'history' of meta-schema for previous versions :(

From Igor #4121-154:

Sorry, I have missed this issue before. The reason is that there are no _SEC_AUTHENTICATION_SYSTEM and _SEC_AUTHENTICATION_DOMAINS tables in a converted database (I understand they always present in the 4GL database). The cleaner solution is just to surround the code inside SecurityOps.readDomains() by try/catch to bypass processing these tables. Should I do this myself and, if so, in which branch?

This is something which I think is expected to be maintained post-conversion. So I don't want to rely on these tables at all, only on what the directory has. But in the mean time, please protect that code so that it does not abend (add a log message that these tables are not present). Commit to 4124a

Constantin Asofiei wrote:

This is something which I think is expected to be maintained post-conversion. So I don't want to rely on these tables at all, only on what the directory has. But in the mean time, please protect that code so that it does not abend (add a log message that these tables are not present). Commit to 4124a

Done in revision 11341.

Igor, does set-client really fail if trying to login while the DB is already connected?

Constantin Asofiei wrote:

Igor, does set-client really fail if trying to login while the DB is already connected?

Sorry, Constantin, I do not understand the question.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Igor, does set-client really fail if trying to login while the DB is already connected?

Sorry, Constantin, I do not understand the question.

This code in SecurityPolicyManager.setClient, with LOGIN state:

         if ((domain instanceof RuntimeDomain) && ConnectionManager.getConnectedDbCount() > 0)
         {
            ClientPrincipal.validationFailed(16388, ConnectionManager.pdbName(1).toStringMessage(), 
                  "Cannot find Domain configuration");
            return new logical(false);
         }

Constantin Asofiei wrote:

This code in SecurityPolicyManager.setClient, with LOGIN state:
[...]
FWD raises this error, but I have a case in a real application where SET-CLIENT is called, but there are connected databases. I've disabled this code for it at this time.

Constantin. As you see this happens when the client domain was created via REGISTER-DOMAIN call. The logic is pretty complicated. Maybe I've incorrectly understood the result of my tests.

• SecurityOps.validateCP and readDomains workarounds
• SecurityPolicyManager.setClient workarounds

These need proper fixes.

Constantin Asofiei wrote:

Igor, when you have a chance, please fix the workarounds in 4124a related to this:

• SecurityOps.validateCP and readDomains workarounds
• SecurityPolicyManager.setClient workarounds

These need proper fixes.

Hi Constantin. Which workarounds do you mean?
Thank you,
Igor.

Igor Skornyakov wrote:

Constantin Asofiei wrote:

Igor, when you have a chance, please fix the workarounds in 4124a related to this:

• SecurityOps.validateCP and readDomains workarounds
• SecurityPolicyManager.setClient workarounds

These need proper fixes.

Hi Constantin. Which workarounds do you mean?

Do a diff of these two files in 4124a, with the base revision (11326). These changes are workarounds, and need proper fixes.

Do a diff of these two files in 4124a, with the base revision (11326). These changes are workarounds, and need proper fixes.

I see, thank you. Is it OK if I will do it tomorrow morning or it is better to do it right now?

Igor Skornyakov wrote:

Do a diff of these two files in 4124a, with the base revision (11326). These changes are workarounds, and need proper fixes.

I see, thank you. Is it OK if I will do it tomorrow morning or it is better to do it right now?

Yes, it can wait for tomorrow; the idea is I want to plan on merging 4124a to trunk soon.

Yes, it can wait for tomorrow; the idea is I want to plan on merging 4124a to trunk soon.

Thank you, it will be done tomorrow.

Constantin,
I've reviewed workarounds I do not think that at this moment I can suggest something really better, mostly because until we'll have support for _SEC_AUTHENTICATION_SYSTEM and _SEC_AUTHENTICATION_DOMAIN tables in FWD it is difficult to say what is a "right" fix, as these tables always present and are non-empty in 4GL databases.

1. I do not understand why your fix in the SecurityPolicyManager.setClient() is needed. Thorough additional testing is required to understand what I've overlooked in my tests. However, this workaround affects only already sealed credential and prevents the false-negative result of the operation. I believe we can leave it as-is for now.

2. The Eugenie's fix in the SecurutyOps.validateCP also looks OK for the reason I've mentioned (no support for mandatory tables). I can suggest some cosmetic improvements to this fix, but they will not change the functionality, so it is better to leave it as-is for now.

Igor Skornyakov wrote:

Constantin,
I've reviewed workarounds I do not think that at this moment I can suggest something really better, mostly because until we'll have support for _SEC_AUTHENTICATION_SYSTEM and _SEC_AUTHENTICATION_DOMAIN tables in FWD it is difficult to say what is a "right" fix, as these tables always present and are non-empty in 4GL databases.

The content of these tables currently is managed 'manually' via the directory.xml; see SecurityOps.readDomains and other related maps/methods. When we need to add support for these tables, we can leverage the existing maps, to i.e. load the data into these maps or load the data from directory into the tables. So, please look into using the existing maps from SecurityOps$WorkArea, instead of relying on these system tables.

1. I do not understand why your fix in the SecurityPolicyManager.setClient() is needed. Thorough additional testing is required to understand what I've overlooked in my tests. However, this workaround affects only already sealed credential and prevents the false-negative result of the operation. I believe we can leave it as-is for now.

This patch was required because the method was being executed while there are DB connections active; why is the ConnectionManager.getConnectedDbCount() > 0 needed for your tests? What is the test that fails if you do not have this check?

2. The Eugenie's fix in the SecurutyOps.validateCP also looks OK for the reason I've mentioned (no support for mandatory tables). I can suggest some cosmetic improvements to this fix, but they will not change the functionality, so it is better to leave it as-is for now.

Again, these mandatory tables are managed via directory.xml - try rewriting this code so that it relies on the directory, and not on reading something from real tables.

Constantin Asofiei wrote:.

I've reviewed workarounds I do not think that at this moment I can suggest something really better, mostly because until we'll have support for _SEC_AUTHENTICATION_SYSTEM and _SEC_AUTHENTICATION_DOMAIN tables in FWD it is difficult to say what is a "right" fix, as these tables always present and are non-empty in 4GL databases.

The content of these tables currently is managed 'manually' via the directory.xml; see SecurityOps.readDomains and other related maps/methods. When we need to add support for these tables, we can leverage the existing maps, to i.e. load the data into these maps or load the data from directory into the tables. So, please look into using the existing maps from SecurityOps$WorkArea, instead of relying on these system tables.

I've ready changed my code and it uses data from the directory.

1. I do not understand why your fix in the SecurityPolicyManager.setClient() is needed. Thorough additional testing is required to understand what I've overlooked in my tests. However, this workaround affects only already sealed credential and prevents the false-negative result of the operation. I believe we can leave it as-is for now.

This patch was required because the method was being executed while there are DB connections active; why is the ConnectionManager.getConnectedDbCount() > 0 needed for your tests? What is the test that fails if you do not have this check?

As far as I remember it was uast/security/set-db-client-test.p. However, as I have already mentioned, the 4GL logic is pretty complicated and my interpretation of the result may be wrong. For example, it is possible that only databases which were open at startup should be considered.

2. The Eugenie's fix in the SecurutyOps.validateCP also looks OK for the reason I've mentioned (no support for mandatory tables). I can suggest some cosmetic improvements to this fix, but they will not change the functionality, so it is better to leave it as-is for now.

Again, these mandatory tables are managed via directory.xml - try rewriting this code so that it relies on the directory, and not on reading something from real tables.

Should I start working on it right now?
Thank you.

Igor Skornyakov wrote:

Again, these mandatory tables are managed via directory.xml - try rewriting this code so that it relies on the directory, and not on reading something from real tables.

Should I start working on it right now?
Thank you.

Yes, I want to stabilize this part, so that at least it behaves consistently (i.e. without persistence dependencies). Don't commit to 4124a, post updates here so I can test them first.

Constantin Asofiei wrote:

Yes, I want to stabilize this part, so that at least it behaves consistently (i.e. without persistence dependencies). Don't commit to 4124a, post updates here so I can test them first.

OK.

Constantin,
Can you please provide a directory.xml file for the scenario which required fixes in question?
Thank you.

See testcases/uast/security/set-db-client.p. It is a simple test demonstrating that the SecurityPolicyManager.setClient workaround (see #3810-352) cannot be 100% correct, at least up to my understanding.

How can I distinguish in FWD between databases connected at startup and those ones which were connected dynamically? It seems that the correct SecurityPolicyManager.setClient workaround should consider only the first ones.
Thank you.

Auto-connected databases are stored in the DatabaseManager.permanentConnections set. This refers to the physical databases (as opposed to logical). You can get an iterator on this set with the method of the same name. If you need a different API, you can add it, but make sure the set cannot be altered through that API.

ConnectionManager.connected contains entries for all connected databases for a session. The keys of this map are the logical names, however the values are ConnectInfo objects, which each contain a reference to the physical database.

Eric Faulhaber wrote:

Auto-connected databases are stored in the DatabaseManager.permanentConnections set. This refers to the physical databases (as opposed to logical). You can get an iterator on this set with the method of the same name. If you need a different API, you can add it, but make sure the set cannot be altered through that API.

ConnectionManager.connected contains entries for all connected databases for a session. The keys of this map are the logical names, however the values are ConnectInfo objects, which each contain a reference to the physical database.

I see. Thank you, Eric.

Igor Skornyakov wrote:

How can I distinguish in FWD between databases connected at startup and those ones which were connected dynamically? It seems that the correct SecurityPolicyManager.setClient workaround should consider only the first ones.
Thank you.

Igor, why do you think SET-CLIENT makes any difference between databases connected at startup compared to the other ones? There are applications where all database connections are done dynamically and this assumption is certainly not correct.

However, if one uses user credentials when connecting the database - either in .pf file or dynamic using CONNECT statement - the database is 'locked' and SET-CLIENT will not attempt to set the identity on this one. The same 'locking' happens when you use SET-DB-CLIENT or SETUSERID, so basically when you call SET-CLIENT progress will call SET-DB-CLIENT using the same client principal on all connected, but not already 'locked' databases.

To 'unlock' a database one need to use SET-DB-CLIENT with an unknown (?) client principal.

Marian Edu wrote:

Igor, why do you think SET-CLIENT makes any difference between databases connected at startup compared to the other ones? There are applications where all database connections are done dynamically and this assumption is certainly not correct.

However, if one uses user credentials when connecting the database - either in .pf file or dynamic using CONNECT statement - the database is 'locked' and SET-CLIENT will not attempt to set the identity on this one. The same 'locking' happens when you use SET-DB-CLIENT or SETUSERID, so basically when you call SET-CLIENT progress will call SET-DB-CLIENT using the same client principal on all connected, but not already 'locked' databases.

To 'unlock' a database one need to use SET-DB-CLIENT with an unknown (?) client principal.

Marian. Thanks a lot for your detailed explanations. The situation in question is very specific. It happens when we try to call SET-CLIENT with a principal which was sealed for a domain which was created using REGISTER-DOMAIN call. The real logic here seems to be very complicated and the behavior depends on many things. My initial implementation which was based on the uast/security/domains.p test which appeared to be incomplete. The fix is based on the uast/security/set-client-test.p.

Please note that the locking logic is not yet implemented as I've got another assignment.

Igor Skornyakov wrote:

Please find the suggested fix for the SecurityPolicyManager.setClient workaround attached. (see #3810-365, #3810-366).

This patch is not OK - the FWD application server will almost always have default databases connected.

Can you describe the test which should show this failure?

Igor Skornyakov wrote:

Please find the suggested fix for the SecurityPolicyManager.setClient workaround attached. (see #3810-365, #3810-366).

This one is not OK, either - the reason is that you do:

      local.get().dbDomains.putIfAbsent(db.toLowerCase(), new HashMap());
      local.get().dbDomainTypes.putIfAbsent(db.toLowerCase(), new HashMap());

      private Domain findDomain(String dbName, String domain)
      {
         Map<String, Domain> dbDomains = null;
         if (dbName != null)
         {
            dbDomains = this.dbDomains.get(dbName.toLowerCase());
         }
         if (dbDomains == null)
         {
            dbDomains = defaultDomains;
         }

         return dbDomains.get(domain.toLowerCase());
      }

Constantin Asofiei wrote:

Igor Skornyakov wrote:

Please find the suggested fix for the SecurityPolicyManager.setClient workaround attached. (see #3810-365, #3810-366).

This patch is not OK - the FWD application server will almost always have default databases connected.

Can you describe the test which should show this failure?

See testcases/uast/security/set-client-test.p

Constantin Asofiei wrote:

Igor Skornyakov wrote:

Please find the suggested fix for the SecurityPolicyManager.setClient workaround attached. (see #3810-365, #3810-366).

This one is not OK, either - the reason is that you do:
[...]
and SecurityOps$WorkArea.findDomain has this:
[...]
This will never fallback to defaultDomains, as WorkArea.dbDomains will have at least an empty map for any database... it will never be null.

The idea is that domains from the databases which are defined on the directory.xml will be loaded before LOAD-DOMAINS will be called and the corresponding entry will exist so there will be no attempt to load from the database. The empty maps are added to avoid multiple attempts to load from the DB which doesn't contain corresponding tables.

Igor Skornyakov wrote:

The idea is that domains from the databases which are defined on the directory.xml will be loaded before LOAD-DOMAINS will be called and the corresponding entry will exist so there will be no attempt to load from the database. The empty maps are added to avoid multiple attempts to load from the DB which doesn't contain corresponding tables.

OK, but this is not working if loading from database fails, and we end up with empty maps - as there is no 'null', it will not fallback to the directory domains.

Constantin Asofiei wrote:

Igor Skornyakov wrote:

The idea is that domains from the databases which are defined on the directory.xml will be loaded before LOAD-DOMAINS will be called and the corresponding entry will exist so there will be no attempt to load from the database. The empty maps are added to avoid multiple attempts to load from the DB which doesn't contain corresponding tables.

OK, but this is not working if loading from database fails, and we end up with empty maps - as there is no 'null', it will not fallback to the directory domains.

It should fallback. The directory domains always take precedence. However, I will double-check tomorrow.

Igor Skornyakov wrote:

See testcases/uast/security/set-client-test.p

I think I understand your confusion. SECURITY-POLICY:SET-CLIENT and SET-DB-CLIENT work with all connected databases (even if you specify a database to SET-DB-CLIENT)! It will go through all connections, and attempt to set the user ID; if it fails, it will not abort, but log an error message (without throwing ERROR condition) and return false.

Also, something else that is missing is this: after you call SECURITY-POLICY:SET-CLIENT, USERID function must return e.g. fwd, for any database for which the SET-CLIENT passed! I see that the SecurityPolicyManager.setClient doesn't do this.

Please fix these two issues; after this, I think we are good.

Also, the SecurityOps patch works OK.

Constantin Asofiei wrote:

I think I understand your confusion. SECURITY-POLICY:SET-CLIENT and SET-DB-CLIENT work with all connected databases (even if you specify a database to SET-DB-CLIENT)! It will go through all connections, and attempt to set the user ID; if it fails, it will not abort, but log an error message (without throwing ERROR condition) and return false.

Also, something else that is missing is this: after you call SECURITY-POLICY:SET-CLIENT, USERID function must return e.g. fwd, for any database for which the SET-CLIENT passed! I see that the SecurityPolicyManager.setClient doesn't do this.

Please fix these two issues; after this, I think we are good.

Also, the SecurityOps patch works OK.

I see, thank you. Can I do it a little bit later? I'm now at the final steps with HttpClient (hope to finish testing main features at this weekend) and prefer not to distract. Is it OK if I will do ut at the beginning of the next week?
Thank you.

Igor Skornyakov wrote:

I see, thank you. Can I do it a little bit later?

Yes, no problem.

Also, I think my statement that USERID function must return e.g. fwd, for any database for which the SET-CLIENT passed! is incorrect. Please do some more tests which include more than one connected database, and call for example l = SET-DB-CLIENT(hcp, "zzz"). (where zzz is an empty database) - this will fail with:

CLIENT-PRINCIPAL validation failed in zzz because - Cannot find Domain configuration (16388)

So, it does matter what the target of set-db-client is; is not pretty clear how SECURITY-POLICY:SET-CLIENT(hcp) chooses the database - is it via the domain configuration?

And something else I noticed: SET-DB-CLIENT and SET-CLIENT must be tested individually, and not in the same program. The behavior is different for SET-DB-CLIENT if SET-CLIENT was called first.

• SET-CLIENT iterates over all connected databases, and processes: the first one matching the domain configuration? Or all matches for the domain configuration?
• SET-DB-CLIENT will:
  • process only the target database, if set-db-client was not called on it (I don't know how it sees that set-db-client was called on it).
  • look through all connected databases, otherwise; but I don't know exactly what is looking for, maybe another domain match?

Igor Skornyakov wrote:

Constantin. I've looked at my code again and now I understand your concern. Thanks for pointing this out. It is fixed now. See the .diff file attached.

This is in 4124a rev 11536.

Constantin Asofiei wrote:

Igor Skornyakov wrote:

I see, thank you. Can I do it a little bit later?

Yes, no problem.

Also, I think my statement that USERID function must return e.g. fwd, for any database for which the SET-CLIENT passed! is incorrect. Please do some more tests which include more than one connected database, and call for example l = SET-DB-CLIENT(hcp, "zzz"). (where zzz is an empty database) - this will fail with:
[...]
and fwd1 will not have the userid set.

So, it does matter what the target of set-db-client is; is not pretty clear how SECURITY-POLICY:SET-CLIENT(hcp) chooses the database - is it via the domain configuration?

And something else I noticed: SET-DB-CLIENT and SET-CLIENT must be tested individually, and not in the same program. The behavior is different for SET-DB-CLIENT if SET-CLIENT was called first.

I think the rules are:

• SET-CLIENT iterates over all connected databases, and processes: the first one matching the domain configuration? Or all matches for the domain configuration?
• SET-DB-CLIENT will:
  • process only the target database, if set-db-client was not called on it (I don't know how it sees that set-db-client was called on it).
  • look through all connected databases, otherwise; but I don't know exactly what is looking for, maybe another domain match?

OK, I will check this. Thank you.

Created task branch 3810d from trunk revision 11330.

Please rebase 3810d. Also, what is the state of this task?

Constantin Asofiei wrote:

Please rebase 3810d. Also, what is the state of this task?

3810d was reabsed to the trunk revision 11335.

I still cannot understand what is wrong with SET-DB-CLIENT support. See also #4124-211

Igor, please see testcases/uast/security/client-principal-multiple-databases.p. Please review it - the idea is that although SET-DB-CLIENT(hcp, "fwd1"). targets a specific database, SECURITY-POLICY:SET-CLIENT(hcp) will iterate through all connected databases and try to authenticate using the specified client principal.

• the .df schema (fwd1.df)
• the data dump for all tables and any system tables (like the domains table)

Constantin Asofiei wrote:

Igor, please see testcases/uast/security/client-principal-multiple-databases.p. Please review it - the idea is that although SET-DB-CLIENT(hcp, "fwd1"). targets a specific database, SECURITY-POLICY:SET-CLIENT(hcp) will iterate through all connected databases and try to authenticate using the specified client principal.

As a side note, when you create a new database, don't upload to the testcases project the binary 4GL database files, but instead upload:

• the .df schema (fwd1.df)
• the data dump for all tables and any system tables (like the domains table)

I see. Thank you, Constantin.

Igor, did you manage to do any progress on the testcase?

Constantin Asofiei wrote:

Igor, did you manage to do any progress on the testcase?

Well, I'm working now on the "lock user" logic on SET-CLIENT. However, your test doesn't explain what was wrong with my understanding of SET-DB-CLIENT after REGISTER-DOMAIN. I have some guesses and checking them now.

• Change the SECURITY-POLICY:SET-CLIENT logic to set db client for all unlocked database and fix the issue with error 16388.
• Add db lock/unlock logic to SET-DB-CLIENT.

This should take about a day with testing.

The SET-CLIENT/SET-DB-CLIENT is re-worked as described in #3810-391.

Committed to 3810d revision 11349.

Setting the testing environment is tricky (see https://proj.goldencode.com/projects/internal-documentation/wiki/4GL_Testcases_Project#Adding-Permanent-Database-Support-Using-H2). The testing will be finished tomorrow.

I've noticed an incompatibility between 4GL and FWD. For the connected databases, 4GL assigns USERID (it is an OS user account if the _User table is empty) while in FWD the USERID function returns an empty string.

I've suddenly started to get the following exception while retrieving data from the _User table:

Caused by: org.h2.jdbc.JdbcSQLException: Column "__IDOMAIN_NAME" not found; SQL statement:
SELECT __iuserid, __idomain_name, password FROM meta_user [42122-197]
        at org.h2.message.DbException.getJdbcSQLException(DbException.java:357)
        at org.h2.message.DbException.get(DbException.java:179)
        at org.h2.message.DbException.get(DbException.java:155)
        at org.h2.expression.ExpressionColumn.optimize(ExpressionColumn.java:150)
        at org.h2.command.dml.Select.prepare(Select.java:858)

The column exists in the table and the SQL query can be executed w/o problems from e.g. h2 console. Any ideas?
Thank you.

Igor Skornyakov wrote:

I've noticed an incompatibility between 4GL and FWD. For the connected databases, 4GL assigns USERID (it is an OS user account if the _User table is empty) while in FWD the USERID function returns an empty string.

This is the same for the client principal returned using GET-DB-CLIENT for that database, but only if the _User table is empty. If the database has users defined but none was specified (using -U/-P on startup or on CONNECT) the USERID is an empty string.

Igor Skornyakov wrote:

I've suddenly started to get the following exception while retrieving data from the _User table:
[...]

The column exists in the table and the SQL query can be executed w/o problems from e.g. h2 console. Any ideas?
Thank you.

Sorry, this is the problem with my test database. Please disregard.

Igor Skornyakov wrote:

I've noticed an incompatibility between 4GL and FWD. For the connected databases, 4GL assigns USERID (it is an OS user account if the _User table is empty) while in FWD the USERID function returns an empty string.

It seems that it was done intentionally. The USEDID function call is converted into SecurityOps.getUserIdFromDB which calls SecurityManagerSecurityOps.getUserIdFromDB which always returns SecurityOps.BLANK_USER. The Javadoc says

    * @return   Always return empty string as the SecurityManager is not database dependent.

However, this behavior is not compatible with 4GL.

Igor Skornyakov wrote:

Igor Skornyakov wrote:

I've noticed an incompatibility between 4GL and FWD. For the connected databases, 4GL assigns USERID (it is an OS user account if the _User table is empty) while in FWD the USERID function returns an empty string.

It seems that it was done intentionally. The USEDID function call is converted into SecurityOps.getUserIdFromDB which calls SecurityManagerSecurityOps.getUserIdFromDB which always returns SecurityOps.BLANK_USER. The Javadoc says
[...]

However, this behavior is not compatible with 4GL.

Well, it seems that it is a configuration issue. However, I do not understand how to configure a non-default value of the userSecurityOpsClass. Please advise.
Thank you,

Igor Skornyakov wrote:

Well, it seems that it is a configuration issue. However, I do not understand how to configure a non-default value of the userSecurityOpsClass. Please advise.
Thank you,

I've found how to configure userSecurityOpsClass. Now USERID returns correct value after successful SET-CLIENT/SET-DB-CLIENT/SETUSERID, but before these calls, it still returns blank user name which is not compatible with 4GL.

I understand that the issue described in #3810-399 is not in the scope of this task.

The branch 3810d is ready for the code review.

Igor Skornyakov wrote:

The branch 3810d is ready for the code review.

Does the #3810-387 testcase work properly now?

Is #3810-399 fixed, in terms of USERID is OK after SET-CLIENT, and not before?

Constantin Asofiei wrote:

Does the #3810-387 testcase work properly now?

Yes, it does.

Is #3810-399 fixed, in terms of USERID is OK after SET-CLIENT, and not before?

No, it is not fixed as I considered this out of the scope of this task. For the full compatibility (before SET-CLIENT/SET-DB-CLIENT/SETUSERID calls), we need at least to add support for _oslocal authentication system.

Branch 3810d was merged into the trunk revision 11338.

Greg Shah wrote:

We actually have much of that code in our spawner today (for Linux/UNIX and Windows). But it will take some time to refactor and implement. For now, please put _oslocal on hold.

Please create a new note that has the full list of work on this task that is not complete. When we come back to this work I want it to be simple to understand what needs to happen.

Igor: Did you ever create this note? I need the list of open items remaining from this task. I think this will include _oslocal, the missing stuff from SSO, the issue in #3810-402 (#3810-399) and testing//fixing/compatibility with all the testcases written for this task. Please include any of these that are open plus any other items that I did not mention.

Greg Shah wrote:

Igor: Did you ever create this note? I need the list of open items remaining from this task. I think this will include _oslocal, the missing stuff from SSO, the issue in #3810-402 (#3810-399) and testing//fixing/compatibility with all the testcases written for this task. Please include any of these that are open plus any other items that I did not mention.

• AUDIT-POLICY resource
• AUDIT-CONTROL resource
• _oslocal authentication system
• _extsso authentication system
• Custom authentication system with callback
• Support of _sec-authentication-domain and _sec-authentication-system tables conversion (?)
• Initial value of USERID (before SET-CLIENT/SET-DB-CLIENT call - #3810-402 (#3810-399)
• CLIENT-PRINCIPAL for multi-tenant databases support (TENANT-ID and TENANT-NAME methods, DB-LIST attribute, (de)serialization of corresponding data)
• Running tests from Marian's team test suite.

• if you try to use set-client a second time, on a already auth DB (with the same client-principal), then FWD fails. I think I know how to fix this.
• I have a problem with validateSeal. When serializing the CLIENT-PRINCIPAL, the domainAccessCode is not serialized (as it should be, as this is the secret key, right?). So, if you try to validate this CLIENT-PRINCIPAL again, with some other key, it will not work... I'm trying to use the bytes returned by the mac(), but:
  • the serialization process loses some attributes (which are LIST)
  • the MAC attribute is also included when computing mac(String) - I assume this should be excluded. Otherwise, I don't see any way to use the MAC (with the new secret key) and compare it with the existing MAC. I'm trying to fix this, but this code seems wrong:
    

                   {
                      bb = new byte[alen];
                      dis.read(bb);
                      readProperties(bb);
                      attributes.put(AttrInfo.PROPS, Optional.of(new ArrayList<String>()));  // here an empty list is added, and not a deserialized list
    

    
    There is a serializeProps, but no deserializeProps API.

If you have any thoughts, please let me know.

Constantin Asofiei wrote:

Igor, I have two issues:

• if you try to use set-client a second time, on a already auth DB (with the same client-principal), then FWD fails. I think I know how to fix this.
• I have a problem with validateSeal. When serializing the CLIENT-PRINCIPAL, the domainAccessCode is not serialized (as it should be, as this is the secret key, right?). So, if you try to validate this CLIENT-PRINCIPAL again, with some other key, it will not work... I'm trying to use the bytes returned by the mac(), but:
  • the serialization process loses some attributes (which are LIST)
  • the MAC attribute is also included when computing mac(String) - I assume this should be excluded. Otherwise, I don't see any way to use the MAC (with the new secret key) and compare it with the existing MAC. I'm trying to fix this, but this code seems wrong:
    [...]
    There is a serializeProps, but no deserializeProps API.

If you have any thoughts, please let me know.

Constantin. I have to recall the details. I will provide my thoughts tomorrow. Please note, however, that some of the issues you've mentioned can be related to the DB-LIST attribute and its (de)serialization, which is not yet implemented (see #3810-406).

Regarding the domainAccessCode. This is indeed a secret data and is not serialized. I'me not sure how a deserialized principal can be validated. May be Marian can explain this?

Igor Skornyakov wrote:

Constantin. I have to recall the details. I will provide my thoughts tomorrow. Please note, however, that some of the issues you've mentioned can be related to the DB-LIST attribute and its (de)serialization, which is not yet implemented (see #3810-406).

Regarding the domainAccessCode. This is indeed a secret data and is not serialized. I'me not sure how a deserialized principal can be validated. May be Marian can explain this?

These part I've solved it, it works after I fixed the PROPS and computing the MAC.

What I still need help with is the SECURITY-MANAGER:SET-CLIENT when all DBs are locked (and I assume at least one is locked with the same CLIENT-PRINCIPAL). Not sure how this can be fixed, I think we still need to check the MAC, but it may be more 'behind-the-scene'. For now, I've disabled the ERROR condition.

Igor Skornyakov wrote:

Regarding the domainAccessCode. This is indeed a secret data and is not serialized. I'me not sure how a deserialized principal can be validated. May be Marian can explain this?

The domain access code definitively should not be serialised, this is used to generate the MAC when the client is sealed and only that MAC needs to be validated when VALIDATE-SEAL is used. The domain access code can be provided to VALIDATE-SEAL, otherwise the AVM uses the one from the domain registry - if any domain that match the one in the CLIENT-PRINCIPAL is available in the registry.

Bref, the key must not be included as it is provided on both operations and if it matches the generated MAC will match as well and that is all is required for the seal to be validated.

Thank you, Marian.

Constantin. I understand that the top priority for me at this moment is #4363 and #4364. Is this correct and can I switch to the problem you've found after tomorrow?
Thank you.

Igor Skornyakov wrote:

Constantin. I understand that the top priority for me at this moment is #4363 and #4364. Is this correct and can I switch to the problem you've found after tomorrow?
Thank you.

Yes, I'm OK for now.

I'm trying to converted run uast/security/export-cp.p with 3809e revision 11456 and two attached databases. The server fails with the following errors:

com.goldencode.p2j.cfg.ConfigurationException:  Initialization failure
    at com.goldencode.p2j.main.StandardServer.hookInitialize(StandardServer.java:2011)
    at com.goldencode.p2j.main.StandardServer.bootstrap(StandardServer.java:966)
    at com.goldencode.p2j.main.ServerDriver.start(ServerDriver.java:483)
    at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:444)
    at com.goldencode.p2j.main.ServerDriver.process(ServerDriver.java:1)
    at com.goldencode.p2j.main.ServerDriver.main(ServerDriver.java:860)
Caused by: java.lang.RuntimeException: com.goldencode.p2j.persist.PersistenceException: Error executing DDL for local/fwd1/meta
    at com.goldencode.p2j.main.StandardServer$11.initialize(StandardServer.java:1215)
    at com.goldencode.p2j.main.StandardServer.hookInitialize(StandardServer.java:2007)
    ... 5 more
Caused by: com.goldencode.p2j.persist.PersistenceException: Error executing DDL for local/fwd1/meta
    at com.goldencode.p2j.persist.meta.MetadataManager.executeDDLScript(MetadataManager.java:612)
    at com.goldencode.p2j.persist.meta.MetadataManager.prepareDatabase(MetadataManager.java:312)
    at com.goldencode.p2j.persist.DatabaseManager.initialize(DatabaseManager.java:1436)
    at com.goldencode.p2j.persist.Persistence.initialize(Persistence.java:881)
    at com.goldencode.p2j.main.StandardServer$11.initialize(StandardServer.java:1211)
    ... 6 more
Caused by: com.goldencode.p2j.MissingDataException: Could not load DDL script com/goldencode/testcases/dmo/_meta/meta_table_ddl.sql
    at com.goldencode.p2j.persist.meta.MetadataManager.executeDDLScript(MetadataManager.java:604)
    ... 10 more

With with 3809e revision 11456 uast/security/export-cp.p reveals multiple incompatibilities in CLIENT-PRINCIPAL EXPORT/IMPORT support. Adding more detailed logging to the test reveals more incompatibilities.

Fixing them now.

Contrary to what is was said in #3810-174, the value of the PRIMARY-PASSPHRASE attribute is not always hidden (replaced with "*") on export. Trying to figure the exact logic.

It seems that multiple changes are required. Which branch should I use to commit? At this moment I'm working with 3809e but have not committed anything so far.
Thank you.

3809e is frozen at this time. Please create a new 3810e branch.

Greg Shah wrote:

3809e is frozen at this time. Please create a new 3810e branch.

Thank you, Greg.
Created task branch 3810e from the trunk rev. 11339

I still have two issues with CLIENT-PRINCIPAL EXPORT/IMPORT support.
1. The quirk which is described in #4108 also affects the IMPORT operation. In some situations, the imported principal returns empty values of character attributes, while in other it works as expected.
2. If we import the principal with a defined LOGIN-EXPIRATION-TIMESTAMP attribute which value is less then the time of the import operation then CP gets the EXPIRED value of the LOGIN-STATE. However, in some cases, the seal data (SEAL-TIMESTAMP and MAC attributes values) are assigned and in some, they remain undefined.

There are many "moving parts" which complicates the "black box testing". I think that I understand the core logic but some corner cases remain unclear.

The extended uast/security/export-cp.p test produces the compatible results in 4GL and FWD branch 3810e rev. 11341.
In particular, the visibility of the character attributes in the imported CLIEBT-PRINCIPAL is the same. This can be related to the issues described in #3810-408. However, I'm now starting to look at these issues more thoroughly.
Please note that I still have no solution for problem #3810-413. Any help will be highly appreciated. Thank you.

Igor Skornyakov wrote:

Please note that I still have no solution for problem #3810-413. Any help will be highly appreciated. Thank you.

Sorry, Igor, I had missed this. Please post the <metadata> section (from within the <schema> section) of your p2j.cfg.xml file. Depending upon which metadata tables are in use in your test project, it might look something like this:

      <metadata name="standard">
         <table name="_db" />
         <table name="_file" />
         <table name="_field" />
         <table name="_index" />
         <table name="_index-field" />
         <table name="_lock" />
         <table name="_user" />
      </metadata>

Eric Faulhaber wrote:

Igor Skornyakov wrote:

Please note that I still have no solution for problem #3810-413. Any help will be highly appreciated. Thank you.

Sorry, Igor, I had missed this. Please post the <metadata> section (from within the <schema> section) of your p2j.cfg.xml file. Depending upon which metadata tables are in use in your test project, it might look something like this:
[...]

Thank you, Eric. My metadata section looks exactly the same. Maybe I have to use some other settings?
Thank you.

Does com/goldencode/testcases/dmo/_meta/meta_table_ddl.sql exist in your application jar file?

Eric Faulhaber wrote:

Does com/goldencode/testcases/dmo/_meta/meta_table_ddl.sql exist in your application jar file?

No, there is no such file.

Igor Skornyakov wrote:

Eric Faulhaber wrote:

Does com/goldencode/testcases/dmo/_meta/meta_table_ddl.sql exist in your application jar file?

No, there is no such file.

Sorry. This file exists.

Well, I understand what was wrong in my setup. I had large_customer_application.jar in my project classpath. Now the server starts OK. Sorry.

With 3810e revision 11341, the SET-DB-CLIENT with the imported sealed principal works fine. So the issue described at the beginning of the note 407 was most likely due to an attribute visibility issues which are fixed now.
However, the situation is "too good" as FWD accepts any sealed principal in a LOGIN state at this point. This is about using MAC which was mentioned in note 407 but in a slightly different sense. It should be easy to fix. Working on this now.

SET-DB-CLIENT for the principal in a LOGIN state validates only DOMAIN-NAME and DOMAIN-ACCESS-CODE (using MAC). The user name and password are ignored.

The issue #3810-407 resolved in 3810e revision 11342.
BTW: it appears the PRIMARY_PASSPHRASE attribute value is ignored in CLIENT-PRINCIPAL MAC calculation.

Igor, I'm OK with the changes. This can be merged with 4335a.

On a side note, in validateSeal I left this comment: TODO: if domainAccessCode is missing, must look through the domain registry...? - is the domainAccessCode parameter mandatory? What happens if this is missing?

Constantin Asofiei wrote:

Igor, I'm OK with the changes. This can be merged with 4335a.

On a side note, in validateSeal I left this comment: TODO: if domainAccessCode is missing, must look through the domain registry...? - is the domainAccessCode parameter mandatory? What happens if this is missing?

Constantin. In 4GL the domainAccessCode parameter is mandatory for the SEAL call.

Igor Skornyakov wrote:

Constantin. In 4GL the domainAccessCode parameter is mandatory for the SEAL call.

And if is an unknown value?

Constantin Asofiei wrote:

Igor, I'm OK with the changes. This can be merged with 4335a.

Constantin. 3810e was created from a relatively fresh version of the trunk. Is it safe (possible) to merge it into 4335a? Maybe it is better to just manually merge the changes as they affect only two classes?
Thank you.

Constantin Asofiei wrote:

And if is an unknown value?

An error 4065 is generated.

Igor Skornyakov wrote:

Maybe it is better to just manually merge the changes as they affect only two classes?

Yes, manually merge the changes.

Another question: I see there is a no-arg CLIENT-PRINCIPAL:VALIDATE-SEAL method. And there is this, https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dvpin%2Fclient-principal-object-methods.html%23wwID0E6LTO, which states:

Do not pass a value and ensure that the session domain registry contains a domain entry matching the security domain defined for the client-principal object by the DOMAIN-NAME attribute and that the entry includes a domain access code whose value is identical to the access code used to seal the object (see Setting up and using domain registries).

Constantin Asofiei wrote:

Igor Skornyakov wrote:

Maybe it is better to just manually merge the changes as they affect only two classes?

Yes, manually merge the changes.

OK. Thank you.

Another question: I see there is a no-arg CLIENT-PRINCIPAL:VALIDATE-SEAL method. And there is this, https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dvpin%2Fclient-principal-object-methods.html%23wwID0E6LTO, which states:

Do not pass a value and ensure that the session domain registry contains a domain entry matching the security domain defined for the client-principal object by the DOMAIN-NAME attribute and that the entry includes a domain access code whose value is identical to the access code used to seal the object (see Setting up and using domain registries).

Oops. It seems that I've overlooked this, sorry. I have to add a corresponding test and update the code. Should I do it right now?
Thank you.

Igor Skornyakov wrote:

Oops. It seems that I've overlooked this, sorry. I have to add a corresponding test and update the code. Should I do it right now?

Yes, please do. And please go through https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dvpin%2Fauthenticating-and-managing-user-identity-in-abl.html%23 and see if there is anything which may be missed (not a feature which is planned to be implemented, but some behaviour which is incomplete in FWD).

Constantin Asofiei wrote:

Yes, please do. And please go through https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dvpin%2Fauthenticating-and-managing-user-identity-in-abl.html%23 and see if there is anything which may be missed (not a feature which is planned to be implemented, but some behaviour which is incomplete in FWD).

OK, thank you. I will first merge into 4335a and continue in it. Is it OK?
Thank you.

Igor Skornyakov wrote:

I will first merge into 4335a and continue in it. Is it OK?

Yes.

Manually merged updates into 4335a revision 11469.

VALIDATE-SEAL is almost fixed, but it appears the LOAD-DOMAINS is broken. Will fix it tomorrow.

VALIDATE-SEAL is fixed in 4335a revision 11475.

Is this ready for a code review?

Greg Shah wrote:

Is this ready for a code review?

I think so.

Constantin: please review.

Review for 4335a rev 11346 - I'm OK with the changes.

Task branch 4335a was merged to trunk as revision 11345.

Is #3810-406 an accurate and complete list of the remaining work in this task?

In #3810-406, you list these:

AUDIT-POLICY resource
AUDIT-CONTROL resource

Are you saying that the items in #3810-1 related to these resources are not implemented?

Or are you saying that the related items in #3810-1 are done, but there remains much of those resources which are unimplemented?

Greg Shah wrote:

Is #3810-406 an accurate and complete list of the remaining work in this task?

In #3810-406, you list these:

AUDIT-POLICY resource
AUDIT-CONTROL resource

Are you saying that the items in #3810-1 related to these resources are not implemented?

Or are you saying that the related items in #3810-1 are done, but there remains much of those resources which are unimplemented?

Yes, the list is accurate. I have not even started to work on AUDIT-* as I have been assigned to another task(s).