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

Use IB CP API latest binaries for Docker builds #209

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

teodorkostov
Copy link

  • Remove IB CP API binaries from repo
  • Add build stage in the Dockerfile(s)
  • Add 172.* addresses to whitelist

- Remove IB CP API binaries from repo
- Add build stage in the Dockerfile(s)
- Add 172.* addresses to whitelist
@teodorkostov
Copy link
Author

Should fix #208. Maybe provide a workaround for #207.

@Voyz
Copy link
Owner

Voyz commented Aug 6, 2024

Hey @teodorkostov thanks for submitting this PR.

I've spent a while reviewing the idea behind your proposed changes - one of having the dependency as latest each time we build.

I find good reasons behind doing both pinned versions and latest:

pinned:

  • better compatibility with our code - we know how the dependency works with our code
  • less errors and surprises - we're less likely to introduce breaking changes
  • lower maintainability cost - we don't need to test each build in case the gateway version increased
  • clear versioning system - we know which IBeam version was shipped with which Gateway

latest:

  • better compatibility with IBKR - pretty self explanatory, code is the latest shipped from IBKR
  • better security - in theory this means we get the security patches from IBKR faster, though in practice I see hardly any meaningful development on the Gateway itself for this to be a contributing factor
  • reduced technical debt - having old dependencies is a not great

However, I don't have time resources to manually verify the integrity of shipped image each time we build should the Gateway version change. I'd also feel uncomfortable publishing an image which people use in production environments with latest dependencies that haven't been at all tested. I feel that going with latest would only make sense if we could lower the maintenance cost of having latest tested every time we build by introducing a testing suite with CI/CD. This is currently not present in IBeam - would you be interested in introducing it?

An opposite solution would be to keep things as they are. IBKR integrates meaningful changes to the gateway infrequently enough that manually updating it every year or two has been sufficient. Do you feel that this has changed now?

@Voyz
Copy link
Owner

Voyz commented Aug 6, 2024

As for:

Add 172.* addresses to whitelist

172.* addresses include both private and public IP addresses, hence hard-coding them as allowed seems to be a security risk and is not advisable:

@teodorkostov
Copy link
Author

teodorkostov commented Aug 7, 2024

Hey, @Voyz.

  • About the IP, you are right. I did not pay attention that the mask was /12. I'll fix that.
  • CI/CD is would be great. However it is not the solution. More important would be to come up with a strategy regarding IBKR binaries management. CI/CD should be a another topic.

Now, let's decide on how to deal with the IBKR code versioning.

The problem with IBKR is that you can get only the latest version from them. Logically, at some point ibeam should get the latest IBKR version. The question is when? I would argue always.

  • Keeping binaries in the source code it is not a good practice. That's why there are package repositories.
  • Automating the task to get the latest binaries solves the barrier of going to the IBKR website, downloading, unzipping, preparing the binaries. Repeatable work should be automated. No one has time for that.
  • If the latest IBKR sources break stuff so be it. The ibeam users would check the latest of your software that is packaged together with IBKR binaries and will just not upgrade.
  • It's OK for breaking changes to happen. This is an open source project with one owner and a handful of maintainers. Not a critical service developed by a multinational software company with thousands of employees.
  • I added the checksum entry so that when a new version from IBKR appears it is immediately visible. Then some more tests could be performed.
  • This is the simplest solution that would require the least amount of effort. If it has a critical flaw that most users cannot live with it could be adjusted.

The other solution would be to create the build image that holds the IBKR binaries separately. Version that image properly. And use that versioned image in the build stage. It requires more effort. Should that effort be invested now? I would argue no. Go with with the simpler solution now. Do a couple of ibeam versions like that. If it becomes an issue then invest the effort in proper versioning of the IBKR code. And who knows, maybe when the need to properly version the IBKR code they could have solved that problem themselves.

@bfoz
Copy link

bfoz commented Aug 11, 2024

For what little it's worth, I mostly agree with @teodorkostov. Occasional breaking changes are irritating, but to be expected. I doubt anyone expects you (@Voyz) to be providing mission-critical levels of reliability for free.

As long as the images are properly tagged, we can always roll back to a previous tag when something breaks.

Can we tag the auto-fetched image as "nightly" or "latest", and only tag tested images with a version?

How does this affect people who are running standalone instead of using the docker image?

@demircancelebi
Copy link

I like the direction this PR introduces. I think I can help with this:

The other solution would be to create the build image that holds the IBKR binaries separately. Version that image properly.

I already created https://github.com/demircancelebi/ibkr-clientportal-archive which should archive the versions and the checksums daily automatically going forward. If you like it you can fork it / change it to your needs.

@Voyz
Copy link
Owner

Voyz commented Aug 19, 2024

Guys, firstly, I wanna say that I value your input and I'm open to this discussion. Nevertheless, I feel I'm missing a crucial point here, and I'd appreciate if you could try to see this perspective:

  1. Unless I'm missing something - there has been zero updates to the Gateway since April 2023. If what I see is true, then nothing will change directly by introducing this PR right now. I've just downloaded the latest stable release of the Gateway and it is the same exact code IBeam already ships with, zero changes. I went through their changelog, no changes either. We're talking about versioning like if there were any meaningful versions, but there essentially aren't.
  2. What's more, IBKR indicates they're looking to introduce the OAuth 2.0 API which will most likely mean the Client Portal Web API - and as such the Gateway - will become obsolete.
  3. What these two points indicate, is that if there are to be any changes to the Gateway, it's very likely these will be sparse and possibly only few before deprecation - if any.

Why are we trying to do this? Just for sake of being hypothetically up to date? I think I'm not making an unreasonable statement when I'd say that automation only makes sense if it really automates something, otherwise it's another redundant moving part. Downloading, unzipping and pushing the latest Gateway once it changes takes me less than 5 minutes a year. I totally have time for that; I sometimes spend hours trying to debug various Issues users submit, hence 5 minutes is nothing and not worth automating at all. Arguably, having this automation maintained is going to be a similar level of negligible effort, yeah, but if it fixes nothing and changes nothing - why would we introduce it?

Or am I missing something crucial here, and this PR does change something important for somebody, other than not keeping binaries in the source code? Honestly, if there's something that this change will fix and help for anyone here I'm totally happy to go ahead - please help me see your perspective. But at this stage I don't understand why would we do it and why are we investing time into this.

@demircancelebi
Copy link

I was thinking about security. In case your account is compromised and the gateway is updated, a malicious actor may introduce unwanted code to the clientportal section. Now the burden to check nothing suspicious going on is on the user, I thought removing that burden and relying on IB would be a good idea.

I also checked the diff and asked Claude to summarize them. I guess these were all intentional:
CleanShot 2024-08-20 at 14 03 56@2x

You are right, clientportal is not updated frequently so I'd say it may not be worth the effort if this was not implemented. Since the PR is already here though, I think it'd be better to merge it.

@Voyz
Copy link
Owner

Voyz commented Aug 21, 2024

@demircancelebi thanks for pointing the security out 👍

Correct me if I'm wrong though: if my account gets compromised, a bad actor could introduce malicious code irrespectively of which method we use to support the Docker image with the Gateway files. To my understanding, providing the Gateway automatically from IBKR doesn't mitigate that risk, does it? If it does, how? A bad actor that has compromised my computer or account can introduce suspicious code after the Gateway is pulled from the IBKR servers, or redirect the download request to their own version of the Gateway. If this is a real concern, in either version - manual or automated - the burden of checking whether the files are not suspicious stays with the user.

Even more, one could make an argument that I could be the malicious actor, in which case it would be unadvisable that one uses this image without checking whether the files are not suspicious. I think that could be a fair concern for some, however it would stay true irrespectively of whether my account is compromised or not, and as such irrespectively of which method is used to provide the Gateway files.

Seeing that you've mentioned this though, is there any way we could mitigate this security risk? If there is, I'd suggest we open a new issue and discuss it there.


As for your diff - yes, all the changes are intentional:

  • I couldn't get it to work locally without uncommenting these lines in the run.bat on a Windows machine. This is irrelevant on the image, which uses the run.sh.
  • The missing files are redundant.
  • The additional allows are there to accommodate using the Gateway within a Docker network.

Given above, I feel that my previous question remains - I don't understand why we would introduce the automatic pull seeing that it introduces no practical changes or improvements, and to my understanding it doesn't mitigate the security risk of having my account compromised. As always, please let me know if I misunderstood something here 👍

@teodorkostov
Copy link
Author

Thanks for the info @Voyz.

  1. What's more, IBKR indicates they're looking to introduce the OAuth 2.0 API which will most likely mean the Client Portal Web API - and as such the Gateway - will become obsolete.

I was not aware that the Client Portal would become obsolete soon. This is certainly a good thing.

The only value this PR has:

  1. Remove the manual step of downloading the CPAPI sources.
  2. Remove the CPAPI sources from the code base.

I am perfectly fine if you would like keep things as is. If you have made up your mind just cancel this PR.

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

Successfully merging this pull request may close these issues.

5 participants