Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create describe function for encoding an decoding #2

Closed
wadearnold opened this issue Jun 14, 2023 · 7 comments
Closed

Create describe function for encoding an decoding #2

wadearnold opened this issue Jun 14, 2023 · 7 comments
Assignees

Comments

@wadearnold
Copy link
Member

wadearnold commented Jun 14, 2023

Like ISO8583 add the describe() function to create a human-readable output of the encoding inputs and outputs.

PIN blocks: PIN block encrypt operation finished
****************************************
PAN:            43219876543210987
PIN:            1234
PAD:            N/A
Format:         Format 0 (ISO-0)
—————————————-
Clear PIN block:0412AC89ABCDEF67
PIN blocks: PIN block decode operation finished
****************************************
PIN block:      0412AC89ABCDEF67
PAN:            43219876543210987
PAD:            N/A
Format:         Format 0 (ISO-0)
—————————————-
Decoded PIN:    1234

Support different formats of the pinblock

PIN blocks: PIN block encrypt operation finished
****************************************
PAN:            432198765432109870
PIN:            1234
PAD:            N/A
Format:         Format 4 (ISO-4)
—————————————-
Clear PIN block:441234AAAAAAAAAA911B9B36BC7CE94E
Clear PAN block:64321987654321098700000000000000
PIN blocks: PIN block decode operation finished
****************************************
PIN block:      441234AAAAAAAAAA911B9B36BC7CE94E
PAN block:      64321987654321098700000000000000
PAD:            N/A
Format:         Format 4 (ISO-4)
—————————————-
Decoded PIN:    1234
Decoded PAN:    432198765432109870
@mfdeveloper508
Copy link
Contributor

PR

@alovak
Copy link
Contributor

alovak commented Jun 15, 2023

Not sure how displaying PIN, PAN and output can be useful.

I see how pin and pan fields (that we calculate with the control char, length, padding, etc.) can be useful when you debug things e.g.:

PIN block:       441234AAAAAAAAAA911B9B36BC7CE94E
PAN block:      64321987654321098700000000000000

but in the #6 , we are not showing it. We are showing only two new data points into addition to what user already has: formatted name and padding.

What is the purpose of such output? I used examples from here to check if I'm doing it right. So, if our goal it to help people debug things, then I see two options for the implementation here:

Option 1 - use io.Writer as a debug output

By default, DebugOutput should be io.Discard, it can be set via constructor or by setting the field on the struct like this:

format := &formats.ISO0{
        DebugOutput: os.Stdout,
}

internally it can write any information that can be used for debugging - which can really be useful

My concern here is the security. It would be pretty bad if someone somehow will output real pins/pans somewhere without the intention to do so.

Option 2 - create methods that will show debug information so we can use them in Describe

It's similar to what we have in the #6 but we should agree on what exactly we want to see and how useful it is

For example, for iso4 we have many intermediate calculations, pin field, encrypted pin field, pan field, xor'ed encrypted pin and pan, encrypted pan (result). Which of them do we really want to show?

Here are two suboptions:

1

we can keep methods private, and move Describe into formats package so it will have access to them

2

agree on the Describable interface and use it instead of adding interface for the format (ISO) (because: define interface where you use it, not where you implement it).

@alovak
Copy link
Contributor

alovak commented Jun 15, 2023

@adamdecaf what do you think about Option 1? Do you think it's ok to allow setting DebugOutput?

@adamdecaf
Copy link
Member

I like Option 1 and the default io.Writer can be nil so that we only write when .DebitWriter != nil. Writing all of that unused debug information isn't great from a privacy/PCI point of view as those strings will reside in memory for a period of time.

@mfdeveloper508
Copy link
Contributor

option 1 is good for me too

@alovak
Copy link
Contributor

alovak commented Jun 16, 2023

Let's think about the API for this. I have following suggestions

Option 1 - separate method to set debug writer + without exporting the field

type ISO0 struct {
    Filler      string
    debugOutput io.Writer
}

/// usage
pinblock := &formats.NewISO0()
pinblock.SetDebugWriter(os.Stdout)

as I have a security concern, the first option seems to be a better choice. When the engineer explicitly calls a method like SetDebugWriter(), it's clear that they're enabling debug logging, which should make them more aware of what they're doing. Also, debugOutput is unexported, which means it cannot be directly accessed outside the package.

Option 2 - just set the field

type ISO0 struct {
    Filler      string
    DebugOutput io.Writer
}

// usage
pinblock := &formats.NewISO0()
pinblock.DebugOutput = os.Stdout

it's more straightforward, but I would prefer the Option 1

@mfdeveloper508 @adamdecaf what is your opinion?

@adamdecaf
Copy link
Member

Go usually avoids setters/getters. If someone calls pinblock.DebugOutput = os.Stdout they know what they are doing. To me that's the same as calling .SetDebugOutput(..).

Option 2 is more the Go standard way of writing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants